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

Should client::conn::http2::SendRequest be Clone? #3036

Closed
vi opened this issue Oct 29, 2022 · 7 comments · Fixed by #3042
Closed

Should client::conn::http2::SendRequest be Clone? #3036

vi opened this issue Oct 29, 2022 · 7 comments · Fixed by #3042
Labels
A-client Area: client. A-http2 Area: HTTP/2 specific. C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Milestone

Comments

@vi
Copy link
Contributor

vi commented Oct 29, 2022

Should hyper::client::conn::http2::SendRequest be Clone, just like h2::client::SendRequest?

@vi vi added the C-feature Category: feature. This is adding a new feature. label Oct 29, 2022
@seanmonstar
Copy link
Member

Hm, probably yes. It should also likely internally use the UnboundedSender (if it isn't already) which allows it to be cloned and so on.

@seanmonstar seanmonstar added A-client Area: client. E-easy Effort: easy. A task that would be a great starting point for a new contributor. A-http2 Area: HTTP/2 specific. labels Nov 2, 2022
@seanmonstar seanmonstar added this to the 1.0 Final milestone Nov 2, 2022
@LucioFranco
Copy link
Member

This should probably extend to h1 as well?

@seanmonstar
Copy link
Member

h1 can't send multiple requests concurrently.

@LucioFranco
Copy link
Member

Right but there is a buffer in-front of the sending task so imo we should model the api like that even if it doesn't send them concurrently?

@seanmonstar
Copy link
Member

I don't believe the h1 side has much of a buffer... I think only a slot of 1, where the SendRequest puts the next request, and then the dispatch task can poll and take it to serialize to IO. Any more of a buffer for h1 should like be done with tower::buffer, I think.

@LucioFranco
Copy link
Member

@seanmonstar so for a thick client, the idea will be that it will add that clone portion on top of the h1 portion to make the h1 and h2 api's match? That was my concern was that these two very (same named) similar types have different implementations.

@seanmonstar
Copy link
Member

If a client wanted to serialize requests over the single connection, they could. What hyper::Client used to do was reuse the same HTTP/2 connection by cloning it, and use new HTTP/1 connections by calling connect().

I don't think at the conn::http1 layer we should be making work exactly the same as conn::http2.

vi added a commit to vi/hyper that referenced this issue Nov 4, 2022
@seanmonstar seanmonstar modified the milestones: 1.0 Final, 1.0 RC2 Nov 8, 2022
vi added a commit to vi/hyper that referenced this issue Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-client Area: client. A-http2 Area: HTTP/2 specific. C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants