Skip to content

Temporarily remove HTTP/2 support with Hyper#12191

Closed
jsha wants to merge 1 commit into
curl:masterfrom
jsha:no-http2-hyper
Closed

Temporarily remove HTTP/2 support with Hyper#12191
jsha wants to merge 1 commit into
curl:masterfrom
jsha:no-http2-hyper

Conversation

@jsha

@jsha jsha commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

The current design of the Hyper integration requires rebuilding the Hyper clientconn for each request. However, building the clientconn requires resending the HTTP/2 connection preface, which is incorrect from a protocol perspective. That in turn causes servers to send GOAWAY frames, effectively degrading performance to "no connection reuse" in the best case. It may also be triggering some bugs where requests get dropped entirely and reconnects take too long.

This doesn't rule out HTTP/2 support with Hyper, but it may take a redesign of the Hyper integration in order to make things work.

Partial fix for #11203

@jsha jsha mentioned this pull request Oct 24, 2023
@jsha jsha force-pushed the no-http2-hyper branch 2 times, most recently from 5216ef2 to d8b89d1 Compare October 24, 2023 20:18
@jsha

jsha commented Oct 24, 2023

Copy link
Copy Markdown
Contributor Author

Having some trouble with the test failures. Here are the tests that are failing:

371 728 1148 1268 1278 1471 1472

When I run them locally, though, they pass (with Hyper enabled).

Five out of those seven verify stderr output. For instance, case 728 when failing in CI shows this diff:

2023-10-24T20:27:32.7126468Z  728: stderr FAILED:
2023-10-24T20:27:32.7168278Z --- log/check-expected	2023-10-24 20:27:32.707749622 +0000
2023-10-24T20:27:32.7169745Z +++ log/check-generated	2023-10-24 20:27:32.707749622 +0000
2023-10-24T20:27:32.7170864Z @@ -1 +1,2 @@
2023-10-24T20:27:32.7172381Z +Warning: option CURLOPT_HTTP_VERSION returned error (1)[LF]
2023-10-24T20:27:32.7173603Z  curl: (97) SOCKS5: the destination hostname is too long to be resolved remotely by the proxy.[LF]

In other words, it expected just the error about SOCKS5 to appear on stderr, but additionally stderr has a warning "option CURLOPT_HTTP_VERSION returned error (1)".

It seems pretty clear the failures are related to this branch (they only show up for the Hyper CI task), but I'm not yet sure what causes them.

@jsha

jsha commented Oct 25, 2023

Copy link
Copy Markdown
Contributor Author

Ok, I got the Linux / hyper build to pass. Now what's failing is AppVeyor / autotools and AppVeyor / winbuild, which I think are flaky? At any rate I think this is ready for review.

@icing

icing commented Oct 26, 2023

Copy link
Copy Markdown
Contributor

Ok, I got the Linux / hyper build to pass. Now what's failing is AppVeyor / autotools and AppVeyor / winbuild, which I think are flaky? At any rate I think this is ready for review.

Yes, the AppVeyor ones are unfortunately flaky. Nothing related to this PR.

@bagder bagder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly that this PR then makes #11203 work "fast", just not using HTTP/2?

While it seems a little like a step backwards, I also realize that the current way we use the hyper API is wrong so this might be appropriate short term fix while we try to get a better understanding how the API should be used.

Nit: add a mention of this to docs/HYPER.md to help the next fellow hyper-in-curl hacker.

@jsha

jsha commented Oct 26, 2023

Copy link
Copy Markdown
Contributor Author

Do I understand it correctly that this PR then makes #11203 work "fast", just not using HTTP/2?

Yep, that's correct.

Nit: add a mention of this to docs/HYPER.md to help the next fellow hyper-in-curl hacker.

Done.

Comment thread docs/HYPER.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
needs changes so that a hyper_clientconn can last for the duration
needs changes so that a `hyper_clientconn` can last for the duration

This should make the spellchecker happy.

The current design of the Hyper integration requires rebuilding the
Hyper clientconn for each request. However, building the clientconn
requires resending the HTTP/2 connection preface, which is incorrect
from a protocol perspective. That in turn causes servers to send GOAWAY
frames, effectively degrading performance to "no connection reuse" in
the best case. It may also be triggering some bugs where requests get
dropped entirely and reconnects take too long.

This doesn't rule out HTTP/2 support with Hyper, but it may take a
redesign of the Hyper integration in order to make things work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants