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

hyper/http 1.0 bumps #1352

Closed
wants to merge 21 commits into from
Closed

hyper/http 1.0 bumps #1352

wants to merge 21 commits into from

Conversation

clux
Copy link
Member

@clux clux commented Nov 20, 2023 •
edited
Loading

WIP for #1351. Still failing. See comments in #1351.

@clux clux mentioned this pull request Nov 20, 2023
Closed
13 tasks
@clux clux added the changelog-change changelog change category for prs label Nov 20, 2023
@clux clux linked an issue Nov 23, 2023 that may be closed by this pull request
Dependency upgrades for hyper and http 1.0 #1351
Closed
13 tasks
@clux clux added the help wanted Not immediately prioritised, please help! label Nov 23, 2023
@aviramha
Copy link
Contributor

Hey @clux - seeing this is help wanted, can you suggest specifics? Do you want someone to take over this PR?

@clux clux added this to the 0.89.0 milestone Jan 21, 2024
clux added 2 commits January 21, 2024 14:44
@clux
cargo upgrade remaining affected crates
434a48d 
rustls also has a breaking change w.r.t. dangerous features - should just exist now.

Signed-off-by: clux <[email protected]>
@clux
partially work through long list of rustls breaking changes..
bed6462 
Signed-off-by: clux <[email protected]>
@clux
Copy link
Member Author

clux commented Jan 21, 2024

@aviramha sorry, have not had time to think about this until today. appreciate the initiative.

after looking at it a bit tonight, i am thinking that it's worrying how the scope of this upgrade keeps growing; more and more libs are piling on breaking changes (e.g. rustls 0.22) - and if we wait for the hole ecosystem (currently missing hyper-socks2) then we might put ourselves in a bad spot.

if you are able to help out on the fringes with hyper-socks2 (like getting a pr in there / establishing a temporary fork) then that could be helpful (because i doubt it can coexist with all the breaking changes).

in the meantime, i plan on spending some time this week to try to get this to compile (insofar as possible - taking out hyper-socks2 if necessary).

@aviramha
Copy link
Contributor

I started by creating an issue to see if author would accept PRs or we need to fork
ark0f/hyper-socks2#13
I'll try to look into the implementation later!

@aviramha
Copy link
Contributor

ark0f/hyper-socks2#14
Can use my fork for the moment, until it's merged :)

@aviramha
Copy link
Contributor

Let me know if any more help is needed!

clux added 2 commits January 22, 2024 15:45
@clux
bring in aviramha's pin of hyper-socks2
4c57bb3 
Signed-off-by: clux <[email protected]>
@clux
starting to think that Incoming and Full is wrong..
ea86d81 
going to experiment some more in this direction tomorrow

Signed-off-by: clux <[email protected]>
@clux
Copy link
Member Author

clux commented Jan 22, 2024

Let me know if any more help is needed!

Thanks a lot for the help! Your PR has at least unblocked it from a dependency standpoint.

Currently the main thing we need to decide is what interface to make work to in terms of the actual Client::inner type.
Before: Buffer<BoxService<Request<Body>, Response<Body>, BoxError>, Request<Body>>
After; not sure.

  • the Response Body needs to be collected and streamed so probably StreamBody with BodyExt::collect
  • the old Body struct used in both the request and response before now corresponds to Incoming.. but you're not meant to construct that, which means it can't be in the Request type (which we do construct)...
  • maybe it's just Bytes or Vec<u8> (what we actually use in the interfaces) if they can be converted to each other..

then in Client::new we need to convert the bodies with the tower layer: MapResponseBodyLayer so it's all a wonderful mess of "body" names - many of which now mean different things 🙃

will be experimenting more with an inner like:

Buffer<BoxService<Request<Bytes>, Response<BodyStream<Bytes>>, BoxError>, Request<Bytes>>

tomorrow..

..but getting some very difficult error messages from client/mod.rs (even in just Client::new), but need to start at the source of the type i think. if you have any hints / ideas / thoughts here then that could be helpful, otherwise i'll try to bang up against the compiler some more tomorrow.

@aviramha
Copy link
Contributor

You're welcome. I appreciate your work :)

I'm going to be a bit radical, which might be because I'm ignorant but why do we need send + inner?

The send abstraction doesn't add much, and I think it might be better if every request would do it's own?

or I think I now understand why we need it, but can't we make the Body generic? or maybe use BoxBody?

@clux
comment out some stuff and relax signatures
d58138a 
trying to focus on the streaming part first

Signed-off-by: clux <[email protected]>
@clux
Copy link
Member Author

clux commented Jan 24, 2024

can't we make the Body generic? or maybe use BoxBody?

Yeah, I am trying to go down a BoxBody abstraction way now, have changed the inner to:

    inner: Buffer<
        BoxService<Request<Bytes>, Response<BodyStream<BoxBody<Bytes, BoxError>>>, BoxError>,
        Request<Bytes>,
    >,

which does get Client::new to compile when relaxing error bounds a bit. It's progressing at snails pace though.

Now.. the problem is more to get the various send callers to get something that works. Have been trying BodyStream as a substitute for Body::wrap_stream(b.into_stream()), but that doesn't actually implement Body so we can't use that in the request_text method.

@clux clux modified the milestones: 0.88.1, 0.89.0 Jan 25, 2024
@clux
Copy link
Member Author

clux commented Feb 12, 2024

ok... i am going to give up on this in its current form. it's too painful for me to overcome the pain threshold of actually battling with what amounts to a few compiles a day with this method so will stop pretending to myself that i'll get to this "any day now" because it's just DoSing myself from working on kube instead.

some things that are problematic that i've found:

imo a less painful way forward for someone could be to make a PoC basic client first (outside the kube codebase) and make that work first with single simple query first, rather than commenting out everything here to make it compile because you just end up substituting one giant confusing compiler error (referencing some trait bound not found 3 steps down the line) for another. then gradually increase complexity, and functionality. it doesn't need to do everything, but at some point it needs to do both basic requests and streams and http ugrades. tower and generic wrappers can come later.

hopefully someone smarter than me can help out with this because i need to step away from this and do some more productive issue management (and this is not a good use of my time atm).

@clux clux closed this Feb 12, 2024
@aviramha aviramha mentioned this pull request Feb 19, 2024
Closed
This was referenced Mar 5, 2024
Closed
Closed
@clux clux removed this from the 0.89.0 milestone Mar 25, 2024
@clux clux deleted the hyper-1.0-upgrades branch October 2, 2024 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
changelog-change changelog change category for prs help wanted Not immediately prioritised, please help!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dependency upgrades for hyper and http 1.0
2 participants
@clux @aviramha