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

H3 http body #734

Merged
merged 17 commits into from
May 4, 2020
Merged

H3 http body #734

merged 17 commits into from
May 4, 2020

Conversation

stammw
Copy link
Contributor

@stammw stammw commented Apr 25, 2020

No description provided.

@djc
Copy link
Member

djc commented Apr 27, 2020

In trying to integrate this with hyper, I think we need to make RecvData and RecvBody public, as well?

@stammw
Copy link
Contributor Author

stammw commented Apr 28, 2020

Yes sure! I'll push that soon.

@stammw stammw force-pushed the h3-http-body branch 2 times, most recently from f170284 to 642cb10 Compare April 29, 2020 05:31
@stammw
Copy link
Contributor Author

stammw commented Apr 29, 2020

In fact RecvData is wrapped into RecvRequest and RecvResponse. Maybe SendData should also be completely wrapped (into SendResponse). For symmetry with SendRequest, but we'll also need this for some control specific to response.

I've got all tests passing into my working copy. I'm currently separating this up into commits. I also have a serious fall in throughput benchmarks I need to solve. I believe it's only the benchmark impl.

Next I'll get the docs a little refresh.

@stammw stammw force-pushed the h3-http-body branch 2 times, most recently from 67f892b to be564af Compare April 29, 2020 21:22
@quentusrex quentusrex mentioned this pull request May 2, 2020
Open
@djc
Copy link
Member

djc commented May 3, 2020

Let's do the docs update in a separate PR, so that I can rebase my pin-project and other tweaks in parallel with your documentation work?

@stammw
Copy link
Contributor Author

stammw commented May 3, 2020 •
edited
Loading

Did you bring more changes? Just 2 failing tests to fix and we're good.

I did not push the last changes.

@djc
Copy link
Member

djc commented May 4, 2020

No, I don't have more changes. Do want to split up my commits a bit more to be a bit cleaner. Thanks for rebasing them into your stack, though!

@stammw
H3: make payload-frames carry a Buf impl
27d8fef 
The HttpBody trait make use of `bytes::Buf` to reference the data. As
the code was holding `Bytes`, a copy would have been necessary.
Introduces a parameter type bound on Buf instead of Bytes for
payload-frames transmits.
@stammw
Copy link
Contributor Author

stammw commented May 4, 2020

So here we are! It's ready for a review. If you want to edit those commits of yours, feel free ! :)

@stammw stammw marked this pull request as ready for review May 4, 2020 07:14
@codecov
Copy link

codecov bot commented May 4, 2020 •
edited
Loading

Codecov Report

Merging #734 into master will increase coverage by 0.40%.
The diff coverage is 66.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   70.63%   71.03%   +0.40%     
==========================================
  Files          72       73       +1     
  Lines       13041    13021      -20     
==========================================
+ Hits         9211     9250      +39     
+ Misses       3830     3771      -59
Impacted Files Coverage Δ
interop/src/main.rs 0.00% <0.00%> (ø)
interop/src/server.rs 0.00% <0.00%> (ø)
quinn-h3/benches/helpers.rs 0.00% <0.00%> (ø)
quinn-h3/src/lib.rs 40.24% <0.00%> (-3.18%) ⬇️
quinn-h3/src/streams.rs 87.09% <ø> (ø)
quinn-h3/src/headers.rs 88.23% <33.33%> (-6.51%) ⬇️
quinn-h3/src/body.rs 44.06% <46.93%> (+11.92%) ⬆️
quinn-h3/src/connection.rs 56.91% <47.05%> (-0.05%) ⬇️
quinn-h3/src/proto/frame.rs 73.41% <50.00%> (-1.16%) ⬇️
quinn-h3/src/data.rs 61.85% <61.85%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca67766...9fcc1a2. Read the comment docs.

@djc djc force-pushed the h3-http-body branch 3 times, most recently from cda5c58 to b24cdbe Compare May 4, 2020 08:25
@djc djc force-pushed the h3-http-body branch 2 times, most recently from b171f0f to 28b259f Compare May 4, 2020 13:51
@djc djc force-pushed the h3-http-body branch from 28b259f to 9fcc1a2 Compare May 4, 2020 14:19
@djc
Copy link
Member

djc commented May 4, 2020

@stammw please have another look. I've spent some time improving things:

  • Keep Body around, making minimal changes to allow it to impl HttpBody
  • Merged commits so that most commits are properly formatted, pass tests and are clippy clean
  • Removed all Unpin bounds (the client-side bounds were still there)
  • Consistently import http_body::Body as HttpBody

I'm still not super-happy with the commit history (the client response cancellation still fails for a bunch of these commits until fixed up in the last one, some commits are very large and not very focused) but I'm also not sure how it could be improved much. If you're happy with my updates, let's merge it.

@stammw
Copy link
Contributor Author

stammw commented May 4, 2020

Pretty neat ! Thanks for the improvements !

@stammw stammw merged commit d45ce73 into master May 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the h3-http-body branch May 4, 2020 15:59
This was referenced May 4, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc 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.
2 participants
@stammw @djc