Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support HTTP2 upgrades with !Send executors #3624

Closed
JanBerktold opened this issue Apr 9, 2024 · 2 comments
Closed

Support HTTP2 upgrades with !Send executors #3624

JanBerktold opened this issue Apr 9, 2024 · 2 comments
Labels
C-feature Category: feature. This is adding a new feature.

Comments

@JanBerktold
Copy link

Is your feature request related to a problem? Please describe.
I'm looking to utilize hyper with gloomio. It already works with either HTTP1 or HTTP2 but does not support upgrades between them.

I've hacked up a dirty example that works (but breaks Send executors) which can be fetched here: JanBerktold/hyper-util@136a2fe

➜  hyper-util git:(master) ✗ cargo run --example single_threaded_server --features=server,http1,tokio,http2
➜  hyper-util git:(not-send) curl -vv --http2 --http2-prior-knowledge http://127.0.0.1:8000
*   Trying 127.0.0.1:8000...
* Connected to 127.0.0.1 (127.0.0.1) port 8000 (#0)
* Using HTTP2, server supports multiplexing
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x55dd9b4a1eb0)
> GET / HTTP/2
> Host: 127.0.0.1:8000
> user-agent: curl/7.81.0
> accept: */*
>
* Connection state changed (MAX_CONCURRENT_STREAMS == 200)!
< HTTP/2 200
< content-type: text/plain
< date: Tue, 09 Apr 2024 20:44:58 GMT
< content-length: 14
<
Hello, world!
* Connection #0 to host 127.0.0.1 left intact

It depends on changes in hyper & HTTP:

  • The main change is in JanBerktold/http@cc1cc35. The Extensions struct requires Send on values (through AnyMap). It stores them as an Any/AnyClone internally and hence can't benefit from automatic Send marking.
  • The change in hyper is straightforward & really just removing Send constraints: JanBerktold@e265150

Now, the main problem AFAICT is that we have to decide for AnyMap to support Send or not with each choice breaking one type of executor. To work around this, I am considering suggesting to add a cargo feature for http to conditionally add the Send constraints.

I am opening this issue for discussion about this option and to brainstorm other potential ones as well as issues that I've missed.

@JanBerktold JanBerktold added the C-feature Category: feature. This is adding a new feature. label Apr 9, 2024
@seanmonstar
Copy link
Member

The changes you've suggest are breaking changes, and with v1.0 out, we won't do that for at least 3 years.

But!

I don't think you need this for what you want. The "upgrades" things you're looking at don't imply HTTP2-Upgrade support. That doesn't actually exist in hyper. You either use "prior knowledge", ALPN, or the hyper-util auto connection which will try to detect the version based on the first few bytes received by the server.

The "upgrades" are more things like WebSockets. And even then, upgrades can be done manually using poll_without_shutdown on the connection types. The "upgrades" extensions make things easier, but required picking either Send or !Send, since it needs to put the IO type in a boxed trait object. But it's not required.

@JanBerktold
Copy link
Author

Wow, I went down the wrong rabbit hole. Thank you for your thorough explanation!

I've added an example here if helpful for others: hyperium/hyper-util#115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature. This is adding a new feature.
Projects
None yet
Development

No branches or pull requests

2 participants