[tracking] support hyper/http/etc. 1.0

[mattklein123]

This is the tracking issue for support for Http & Hyper 1.0

Describe the feature

There may already be a tracking issue on this (I couldn't find it), but was hoping to understand the timeline of supporting the entire hyper 1.0 cascade throughout the ecosystem. For examples functions like: https://docs.rs/aws-smithy-types/latest/aws_smithy_types/body/struct.SdkBody.html#method.from_body_0_4 are sprinkled around and would need to hopefully support 1.0 variants.

Checklist

• [x] https://github.com/smithy-lang/smithy-rs/issues/3365
• [x] Construct SdkBody from http-body = 1.0 bodies (https://github.com/smithy-lang/smithy-rs/pull/3300)
• [ ] Construct SdkBody from http-body = 1.0 bodies that are !Sync via an internal mutex
• [x] #1046
• [x] SdkBody natively implements http-body 1.0 with 0.4x behavior implemented as a shim.
• [ ] Internal code works based on http-body 1.0 frames instead of http-body 0.4.x
• [ ] hyper_ext_1_0 added utilizing Hyper 1.0
• [ ] Default clients use Hyper 1.0 (behavior major version)
• [ ] https://github.com/smithy-lang/smithy-rs/issues/3367
• [ ] #1089
• [ ] #1090

A note for the community

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue, please leave a comment

[jdisanti]

It's definitely on our radar, and we played around with the release candidates before 1.0 was cut. Previous work was tracked in https://github.com/smithy-lang/smithy-rs/issues/1925, but this issue is better for visibility.

[jdisanti]

This is blocking use of the SDK with Axum 0.7 for streaming uploads to S3.

Mentioned in https://github.com/awslabs/aws-sdk-rust/discussions/989

[mattklein123]

This is blocking use of the SDK with Axum 0.7 for streaming uploads to S3.

This is actually my exact use case. :)

[jdisanti]

It looks like you can use the tower-hyper-body-compat crate to convert Axum's http 1.x body into the http 0.4 body that the SDK needs as a workaround. Although, that crate needs to be updated to the full 1.x version instead of the release candidates.

[mattklein123]

It looks like you can use the tower-hyper-body-compat crate to convert Axum's http 1.x body into the http 0.4 body that the SDK needs as a workaround. Although, that crate needs to be updated to the full 1.x version instead of the release candidates.

OK thanks will keep an eye on that. At this point given how disruptive this change is (why?), I'm waiting for things to settle a bit. :)

[rcoh]

We'll have a fix for the specific task of creating a body from an Http 1x body out soon: https://github.com/smithy-lang/smithy-rs/pull/3300

[rcoh]

this is now available: https://docs.rs/aws-smithy-types/latest/aws_smithy_types/body/struct.SdkBody.html#method.from_body_1_x

[mattklein123]

this is now available: https://docs.rs/aws-smithy-types/latest/aws_smithy_types/body/struct.SdkBody.html#method.from_body_1_x

@rcoh I've been picking away at the upgrade in a branch and tried to use this. Unless I'm missing something this doesn't work for the axum use case. For reasons I'm not sure about the axum body is not Sync (it's UnsyncBoxBody). The bounds on this method require a Sync body. Can the bounds be relaxed here to remove Sync?

[rcoh]

The bounds on this method require a Sync body. Can the bounds be relaxed here to remove Sync?

Hmm, possibly. I believe we could create an internal variant that wrapped the body in a tokio::sync::Mutex which would make it sync again. SdkBody is Sync currently so we can't take that bound away without causing breaking changes.

Since the mutex would be uncontended, I don't expect it to have any performance implications.

I've added another checklist item at the top.

[mattklein123]

I've added another checklist item at the top.

OK thanks. It's unfortunate a mutex would need to be added here but IMO it would be better to have this internal to the SDK. I suppose we could ask the axum people to add sync, but I doubt that would get much traction either, even though in practice I would imagine all of the bodies are actually sync.

[rcoh]

In the meantime, you could work around this by creating your own sync-body-wrapper that acquires a mutex when calling poll.

[mattklein123]

In the meantime, you could work around this by creating your own sync-body-wrapper that acquires a mutex when calling poll.

@rcoh I went to take a crack at this and FWIW I don't think a trivial wrapper is possible. The Body trait has functions that are not async (is_end_stream and size_hint), so using an async mutex is not going to work for directly accessing the underlying body in these scenarios. So I suppose the option is to synthesize is_end_stream and size_hint in the context of poll_frame(), or just give up and have some type of intermediate stream that moves data from one body to the other. I will take a quick crack at the former and post the code if I get it working.

[rcoh]

Oh that makes sense. I think we can actually use a regular mutex possibly? Since it isn't held across an await point? I'd need to dig in to understand more.

On Tue, Jan 16, 2024, 6:32 PM Matt Klein @.***> wrote:

In the meantime, you could work around this by creating your own sync-body-wrapper that acquires a mutex when calling poll.

@rcoh https://github.com/rcoh I went to take a crack at this and FWIW I don't think a trivial wrapper is possible. The Body trait has functions that are not async (is_end_stream and size_hint), so using an async mutex is not going to work for directly accessing the underlying body in these scenarios. So I suppose the option is to synthesize is_end_stream and size_hint in the context of poll_frame(), or just give up and have some type of intermediate stream that moves data from one body to the other. I will take a quick crack at the former and post the code if I get it working.

— Reply to this email directly, view it on GitHub https://github.com/awslabs/aws-sdk-rust/issues/977#issuecomment-1894686396, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADYKZ2LX36QLR73UGPYP4DYO4EYZAVCNFSM6AAAAAA76NWY5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJUGY4DMMZZGY . You are receiving this because you were mentioned.Message ID: @.***>

[mattklein123]

Oh that makes sense. I think we can actually use a regular mutex possibly? Since it isn't held across an await point? I'd need to dig in to understand more.

I don't think a regular mutex is sound, because technically the mutex has to be held while calling poll_frame which is an await point. Mostly for fun I tried to implement this and it's actually fairly tricky. This is the best I could come up with without introducing unsafe code: https://gist.github.com/mattklein123/930df76886a7324e291abf0b2003549b.

In particular we have to use Arc<Mutex> so we can get an OwnedMutexGuard as there is no way to prove that the guard lifetime is safe. This could be fixed with unsafe code.

I have no experience writing manual futures so it's possible someone will come along and provide a simpler/faster solution.

[mattklein123]

For anyone watching I updated https://gist.github.com/mattklein123/930df76886a7324e291abf0b2003549b to use unsafe and remove extra arc/box/allocations. I believe it is correct but will test it out more thoroughly.