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

hyper v1 upgrade #357

Merged
merged 1 commit into from
Apr 5, 2024
Merged

hyper v1 upgrade #357

merged 1 commit into from
Apr 5, 2024

Conversation

zuisong
Copy link
Contributor

@zuisong zuisong commented Mar 22, 2024

No description provided.

@zuisong zuisong force-pushed the upgrade-hyper branch 7 times, most recently from 81511aa to 4c467a7 Compare March 22, 2024 16:46
@zuisong
Copy link
Contributor Author

zuisong commented Mar 22, 2024

two test cases that cannot pass, I have already submitted an issue to reqwest.
seanmonstar/reqwest#2201

@zuisong zuisong force-pushed the upgrade-hyper branch 2 times, most recently from a885183 to f46132c Compare March 23, 2024 15:04
Cargo.toml Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
tests/server/mod.rs Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
@zuisong zuisong force-pushed the upgrade-hyper branch 3 times, most recently from 76d8091 to 4473d64 Compare March 25, 2024 14:25
@zuisong zuisong marked this pull request as ready for review March 25, 2024 14:25
@blyxxyz
Copy link
Collaborator

blyxxyz commented Mar 25, 2024

xh does send Content-Length now even for GET requests. That seems weird since RFC 9110 says

A user agent SHOULD NOT send a Content-Length header field when the request message does not contain content and the method semantics do not anticipate such data.

It seems to be a bug? See seanmonstar/reqwest#2202 (comment). Notably this is causing real-world issues for someone, so we shouldn't send this header for GET requests if we can avoid it.

Otherwise we'd have to add it to the request headers that we print.

Test case

Run this in one terminal:

while nc -l -p 8080; do echo; done

Then run this in another:

cargo run -- -v :8080

This is what xh claims to send:

GET / HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Connection: keep-alive
Host: localhost:8080
User-Agent: xh/0.21.0

This is what's actually received:

GET / HTTP/1.1
Accept-Encoding: gzip, deflate, br
User-Agent: xh/0.21.0
Connection: keep-alive
Accept: */*
Host: localhost:8080
Content-Length: 0

@zuisong zuisong marked this pull request as draft March 26, 2024 01:08
@zuisong
Copy link
Contributor Author

zuisong commented Mar 26, 2024

Thanks for pointing that out,I've submitted a PR to fix it.
seanmonstar/reqwest#2207

@blyxxyz
Copy link
Collaborator

blyxxyz commented Mar 26, 2024

This is very odd, but with your patched reqwest xh seems to send a body of 0 by default, i.e. the text 0. This is what I receive through netcat from xh post :8080:

POST / HTTP/1.1
Accept-Encoding: gzip, deflate, br
User-Agent: xh/0.21.0
Connection: keep-alive
Accept: */*
Host: localhost:8080
Transfer-Encoding: chunked

0

(or escaped: POST / HTTP/1.1\r\nAccept-Encoding: gzip, deflate, br\r\nUser-Agent: xh/0.21.0\r\nConnection: keep-alive\r\nAccept: */*\r\nHost: localhost:8080\r\nTransfer-Encoding: chunked\r\n\r\n0\r\n\r\n)

It doesn't happen if I comment out the reqwest patch in Cargo.toml.

(EDIT: this happens because reqwest chooses to use chunked transfer encoding.)


I've also noticed that with hyper v1 xh now errors if you override the Content-Length to be too large, e.g. xh post :8080 Content-Length:10. I don't think that's a big deal though, definitely not blocking for this PR.

@zuisong
Copy link
Contributor Author

zuisong commented Mar 26, 2024

It's really weird. I'll keep looking for a solution.

@zuisong
Copy link
Contributor Author

zuisong commented Mar 28, 2024

The issue is fixed now, could you check if there are other issues?

@zuisong zuisong marked this pull request as ready for review March 28, 2024 04:00
@zuisong zuisong marked this pull request as draft March 28, 2024 04:01
@blyxxyz blyxxyz self-requested a review March 28, 2024 09:56
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks good!

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@zuisong
Copy link
Contributor Author

zuisong commented Apr 5, 2024

Reqwest just released a new version 0.12.3, so this PR can be ready to go

@ducaale ducaale merged commit 4dc5588 into ducaale:master Apr 5, 2024
9 checks passed
@zuisong zuisong deleted the upgrade-hyper branch April 5, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants