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

Decompose Client into utils #3080

Open
4 tasks
seanmonstar opened this issue Dec 9, 2022 · 6 comments · May be fixed by hyperium/hyper-util#31
Open
4 tasks

Decompose Client into utils #3080

seanmonstar opened this issue Dec 9, 2022 · 6 comments · May be fixed by hyperium/hyper-util#31
Labels
A-client Area: client. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. K-hyper-util Crate: hyper-util

Comments

@seanmonstar
Copy link
Member

seanmonstar commented Dec 9, 2022

Besides a connector and pool, the Client from 0.14.x did many small things that should be decomposed into pieces in hyper-util, so users can choose to compose the parts they want.

  • Create a SetHost service layer (could also fit in tower-http) (0.14.x code)
  • Create a Http1RequestTarget service layer (0.14.x code)
  • Create a DelayedRelease service layer (0.14.x code)
  • Create an AutoConnection which can wrap HTTP/1 and HTTP/2 (and eventually 3) connections
@seanmonstar seanmonstar added A-client Area: client. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. K-hyper-util Crate: hyper-util labels Dec 9, 2022
@seanmonstar seanmonstar added this to the hyper-util 0.1 milestone Dec 9, 2022
@seanmonstar seanmonstar removed this from the hyper-util 0.1 milestone Feb 16, 2023
@seanmonstar
Copy link
Member Author

@davidpdrsn hey! any thoughts about some of these things moving to tower-http? Or should they stay as hyper-util? Or a 3rd option?

@tomkarw
Copy link
Contributor

tomkarw commented Jul 28, 2023

Is this issue up to date? If yes, I'd be happy to pick it up.

As for the tower-http part, I don't think it's a blocker for now, we can always move that part of code to a different crate once it's extracted.

@seanmonstar
Copy link
Member Author

I believe so, yea!

@tomkarw
Copy link
Contributor

tomkarw commented Aug 7, 2023

@seanmonstar How should we deal with information that was available in the broad context of send_request function, but won't be available now in the narrower context of the middleware?

Both Http1RequestTarget and DelayedResponse requires pooled (some context about the already established connection) which is not trivially available inService middleware.

@seanmonstar
Copy link
Member Author

@tomkarw Do you mind expanding on what you think is needed? Such as the is_proxied part? I've been thinking that could go somewhere else and not be part of the connector anymore. Possibly a request extension...

@tomkarw
Copy link
Contributor

tomkarw commented Jan 26, 2024

Current v0.14.x Client had following logic: if the connection info indicated that the request is being proxied, and the schema was exactly HTTPS, we normalize the URI to the origin form (stripping the host). When making Http1RequestTarget a Service, we need to work with an interface that only exposes Request. We loose this tight coupling with the connection pool and thus connection info.

We need to decide whether we:

  • remove the proxy case, potentially breaking proxied requests
  • somehow sneak the connection info as part of the Request (like in the extentions?)
  • do what's currently in the PR, which is to set is_proxied when instanciating the Service, but that doesn't work, as the is_proxied per-connection (and thus per-request) information and should be determined separetly for each request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-client Area: client. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. K-hyper-util Crate: hyper-util
Projects
No open projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

2 participants