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.

Sign up for GitHub

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

Jump to bottom

feat(http1): Add support for writing Trailer Fields #3375

Merged
merged 5 commits into from
Dec 15, 2023
Merged

feat(http1): Add support for writing Trailer Fields #3375

merged 5 commits into from
Dec 15, 2023

Conversation

hjr3
Copy link
Contributor

@hjr3 hjr3 commented Oct 26, 2023 •
edited
Loading

Closes #2719

hjr3
hjr3 commented Oct 26, 2023
View reviewed changes
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
@hjr3 hjr3 changed the title feat(http1) Add support for Trailer Fields feat(http1) Add support for writing Trailer Fields Nov 4, 2023
@hjr3 hjr3 marked this pull request as ready for review November 4, 2023 12:44
tottoto
tottoto reviewed Nov 10, 2023
View reviewed changes
Copy link
Contributor

@tottoto tottoto left a comment

Choose a reason for hiding this comment

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

Thanks. It looks good to me. I left some comments.

src/proto/h1/conn.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Show resolved Hide resolved
src/proto/h1/role.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
@hjr3
fix(http1): code review fixes
401dfaf 
- use more idiomatic expressions
- add TE as invalid header
- add tests for encode_trailers
- fix bug in encode_trailers when buffer is empty
hjr3
hjr3 commented Nov 11, 2023
View reviewed changes
Copy link
Contributor Author

@hjr3 hjr3 left a comment

Choose a reason for hiding this comment

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

Thank you the feedback. I fixed most suggestions and explained my use of drain.

src/proto/h1/encode.rs Outdated Show resolved Hide resolved
src/proto/h1/conn.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Show resolved Hide resolved
src/proto/h1/role.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
tottoto
tottoto reviewed Nov 11, 2023
View reviewed changes
src/proto/h1/conn.rs Outdated Show resolved Hide resolved
src/proto/h1/role.rs Outdated Show resolved Hide resolved
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
@hjr3
fix(http1): code review fixes
9cc26a5 
- remove uncessary if statement
- prefer into_iter() instead of drain()
- prefer !vec.is_empty() to vec.len() > 0
- add another test for multiple trailer headers in single response
tottoto
tottoto reviewed Nov 11, 2023
View reviewed changes
src/proto/h1/encode.rs Outdated Show resolved Hide resolved
@hjr3
fix(http1): code review fixes
7a8eb33 
- remove unnecessary into_iter
@tottoto
Copy link
Contributor

tottoto commented Nov 11, 2023

@hjr3 Thank you! Looks good from my point of view.

@tottoto
Copy link
Contributor

tottoto commented Nov 11, 2023

According to the ci log, the miri test ci seems to fail inside http crate which version is 0.2.10. It might not to be caused by this pull request as testing with h2 0.2.9 succeed.

@hjr3
Copy link
Contributor Author

hjr3 commented Nov 11, 2023

According to the ci log, the miri test ci seems to fail inside http crate which version is 0.2.10. It might not to be caused by this pull request as testing with h2 0.2.9 succeed.

I noticed this as well. The write headers function looks pretty basic so I figured it would be upstream.

@hjr3 hjr3 mentioned this pull request Nov 12, 2023
Closed
@seanmonstar
Copy link
Member

Excellent work, and thanks for the review too! I aim to release 1.0 on Wednesday, and then merge this after that dust settles, just to prevent introducing any bugs at the last second.

@seanmonstar seanmonstar changed the title feat(http1) Add support for writing Trailer Fields feat(http1): Add support for writing Trailer Fields Dec 11, 2023
seanmonstar
seanmonstar approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks again! 🥳

@seanmonstar seanmonstar merged commit 31b4180 into hyperium:master Dec 15, 2023
19 checks passed
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024
@hjr3 @0xE282B0
feat(http1): Add support for sending HTTP/1.1 Chunked Trailer Fields (h…
1eec538 
…yperium#3375)

Closes hyperium#2719
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 16, 2024
@hjr3 @0xE282B0
feat(http1): Add support for sending HTTP/1.1 Chunked Trailer Fields (h…
67797f3 
…yperium#3375)

Closes hyperium#2719

Signed-off-by: Sven Pfennig <[email protected]>
@hjr3 hjr3 deleted the http1-trailers branch April 13, 2024 09:16
@hjr3 hjr3 mentioned this pull request Apr 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tottoto tottoto tottoto left review comments

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sending HTTP/1.1 trailers
3 participants
@hjr3 @tottoto @seanmonstar