Upgrade to hyper 1.0.0 #7
Conversation
src/body.rs
Outdated
| unsafe { | ||
| std::mem::transmute::<http_1::HeaderMap, http_02::HeaderMap>(h) | ||
| } |
There was a problem hiding this comment.
Yeah we can't have any transmutes here 😅 Just have to convert one type into the other by copying the data.
There was a problem hiding this comment.
Yeah this was originally just a temporary hack. However, I am struggling to actually remove it. I pushed up a commit which kind of works, but its fairly terrible. Most notably the extension part of Request/Response doesn't actually seem convertable, unless I am missing some way.
The rest did work though, although the conversion is pretty terrible. Likely can use some improvements there, I don't think its robust/correct.
I pushed up another commit with these changes to iterate on
There was a problem hiding this comment.
However, I am struggling to actually remove it.
It might compile but I really doubt it actually works. The memory layout for Extensions for example has changed since it now requires Clone. That'll most likely cause nasty UB.
Most notably the extension part of Request/Response doesn't actually seem convertable, unless I am missing some way
I don't think there is a way to convert extensions since you cannot iterate all the values :/ I'm not sure there is much we can do about it so we have to do some testing and see how much that breaks. I'm pretty sure its gonna break web sockets because they rely on the OnUpgrade extension which is set by hyper and would thus be removed when converting to http 0.2 extensions.
There was a problem hiding this comment.
What do you think is the path forward here?
Seems like our options are:
- transmute - unsafe and almost certainly broken. Definitely don't like this approach
- Convert what we can - should work for some cases but break others
- Change
httpdirectly (possibly in both versions?) to expose things we need - Do nothing; wait for rest of ecosystem to catch up to hyper 1.0
Not sure about others but I am personally only using this for tonic. (2) seems viable enough for that use case (although it wasn't extensively tested yet). (4) also might be fine as well, assuming tonic adds hyper 1.0 support in the coming weeks/months.
There was a problem hiding this comment.
OK I took a stab at it. Not great but best I could come up with. Can probably improve a bit, but for Extension we would need changes to http
(cherry picked from commit daef143)
|
@davidpdrsn I think this is ready to go now, LMK what you think. |
|
Since tonic now supports hyper 1.0 directly, I don't personally have a need for this. If anyone its interested in taking it over please feel free. Thanks! |
Update to hyper 1.0.0.
This change is non trivial due to the bumping of
http. To handle this, this PR introduces conversion functions for http0.2<->http1.0.TODO:
main, not sure if they are broken or its an issue on my sidecc @davidpdrsn @LucioFranco