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

[tracking] support hyper/http/etc. 1.0 #977

Open
4 of 11 tasks
mattklein123 opened this issue Nov 28, 2023 · 15 comments
Open
4 of 11 tasks

[tracking] support hyper/http/etc. 1.0 #977

mattklein123 opened this issue Nov 28, 2023 · 15 comments
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue

Comments

@mattklein123
Copy link

mattklein123 commented Nov 28, 2023 •
edited by Velfi
Loading

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

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
@mattklein123 mattklein123 added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Nov 28, 2023
@jdisanti
Copy link
Contributor

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

@jdisanti jdisanti removed the needs-triage This issue or PR still needs to be triaged. label Nov 28, 2023
@jdisanti
Copy link
Contributor

jdisanti commented Dec 4, 2023 •
edited
Loading

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

Mentioned in #989

@mattklein123
Copy link
Author

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

This is actually my exact use case. :)

@jdisanti
Copy link
Contributor

jdisanti commented Dec 4, 2023

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
Copy link
Author

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 rcoh self-assigned this Dec 8, 2023
@jmklix jmklix added the p2 This is a standard priority issue label Dec 8, 2023
@rcoh
Copy link
Contributor

rcoh commented Dec 11, 2023

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

@rcoh
Copy link
Contributor

rcoh commented Jan 10, 2024

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

@mattklein123
Copy link
Author

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?

@Nugine Nugine mentioned this issue Jan 15, 2024
Closed
@rcoh
Copy link
Contributor

rcoh commented Jan 16, 2024

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
Copy link
Author

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 rcoh changed the title support hyper/http/etc. 1.0 [tracking] support hyper/http/etc. 1.0 Jan 16, 2024
@rcoh
Copy link
Contributor

rcoh commented Jan 16, 2024

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

@cayman-amzn cayman-amzn mentioned this issue Jan 16, 2024
Open
@mattklein123
Copy link
Author

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
Copy link
Contributor

rcoh commented Jan 17, 2024 via email

@mattklein123
Copy link
Author

mattklein123 commented Jan 17, 2024 •
edited
Loading

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 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
Copy link
Author

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.

@rcoh rcoh mentioned this issue Jan 24, 2024
Closed
@tasn tasn mentioned this issue Jan 28, 2024
Closed
@jmklix jmklix mentioned this issue Feb 13, 2024
Closed
@jdisanti jdisanti mentioned this issue Feb 19, 2024
Open
@nickbabcock nickbabcock mentioned this issue Feb 22, 2024
Merged
@BugenZhao BugenZhao mentioned this issue Apr 4, 2024
Open
@goabv goabv unassigned rcoh May 6, 2024
@Velfi Velfi removed their assignment May 9, 2024
@Velfi Velfi removed their assignment Jul 1, 2024
@sergiimk sergiimk mentioned this issue Jul 17, 2024
Merged
@rdettai rdettai mentioned this issue Jul 29, 2024
Open
@aajtodd aajtodd mentioned this issue Sep 9, 2024
Merged
github-merge-queue bot pushed a commit to smithy-lang/smithy-rs that referenced this issue Sep 11, 2024
@aajtodd
deprecate http-02x presign APIs (#3823)
db1a9f1 
## Motivation and Context
* #1925
* awslabs/aws-sdk-rust#977

## Description
Deprecate http-02x APIs from inlineable `PresignedRequest` API. These
should have been feature gated originally but they weren't. For now
we'll mark them deprecated and encourage people to move to the 1.x
equivalents.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x ] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
aws-sdk-rust-ci pushed a commit that referenced this issue Sep 17, 2024
@aajtodd @aws-sdk-rust-ci
[smithy-rs] deprecate http-02x presign APIs (#3823)
af04392 
## Motivation and Context
* smithy-lang/smithy-rs#1925
* #977

## Description
Deprecate http-02x APIs from inlineable `PresignedRequest` API. These
should have been feature gated originally but they weren't. For now
we'll mark them deprecated and encourage people to move to the 1.x
equivalents.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x ] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@aajtodd aajtodd mentioned this issue Oct 7, 2024
Merged
aajtodd added a commit to smithy-lang/smithy-rs that referenced this issue Oct 14, 2024
@aajtodd
consolidate http client implementation(s) (#3866)
e616942 
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->

* #3710
* #1925
* awslabs/aws-sdk-rust#977

## Description
This is the first of what I'm going to assume will be several PRs on the
path to migrating us to hyper 1.x. The goal is to reach a desired state
as far as crate organization, feature flags, and how this is all enabled
by default (eventually). This first PR just moves existing HTTP client
support around as prep work for what's to come.

NOTE: This is all getting merged into a staging branch `hyper1` rather
than `main`

* Migrate `hyper-0.14` and `hyper-1.x` support into a new
`aws-smithy-http-client` crate.
    * re-export `hyper 0.14` from `aws-smithy-runtime`. 
* Remove support from `aws-smithy-experimental` for hyper 1.x and leave
breadcrumb for where it lives now.
* Migrate `CaptureSmithyConnection` into `aws-smithy-runtime-api` so
that it can be used by new crate without creating a circular dependency.


Why a new crate? 

Several reasons:
* The entire hyper and rustls ecosystem bifurcates on hyper 0.x. The
resulting `Cargo.toml` is kind of a mess as a result. In a new crate we
can isolate this ugliness as well as manage feature flags more
meaningfully with this in mind.
* Placing the HTTP client implementation in `aws-smithy-runtime` makes
it a load bearing crate for all of the HTTP and TLS related feature
flags we may wish to expose _in addition to it's own feature flags_.
This sort of explodes with the aforementioned bifurcation.
* I expect `aws-smithy-runtime` and generated SDKs to choose a default
configuration but customers can pull in this new
`aws-smithy-http-client` crate and enable different flags for specific
use cases (e.g. FIPS).
* A new crate may be a good place to explore new functionality (e.g.
we've considered forking our own implementation of `hyper-util` legacy
client, this gives us a natural place for that should we go down that
route)

## Future

Where are we going?

* I want to relocate all of
`aws-smithy-runtime/src/client/http/test_util` into the
`aws-smithy-http-client` crate. These are utilities for testing with a
mock/fake HTTP client and they make more sense in this new crate. This
also allows us to update the utils to support the hyper/http 1.x
ecosystem and feature gate the legacy ecosystem. We can re-export the
legacy ecosystem test support from `aws-smithy-runtime` for now.
* Update our internal usage of these test utils with the new 1.x
ecosystem and deprecate the old APIs but leave them in place.
* I expect we'll deprecate them with a plan to remove or at least
feature gate differently in the future with a recommendation to upgrade.
* I want to explore the tickets/use cases people have asked for to see
what/if we can do anything better with the hyper 1.x API surface. In
particular I think we may want to just expose our own builder type (new
type around hyper-util builder).
* Because `hyper-util` is not 1.x we can't expose the
`HyperClientBuilder` like we did previously. I don't think we should
even if it was 1.x though, we should offer a "default client" with knobs
for all the things we do want to support directly. Anything not found
there you have to bring your own client configured to your heart's
content.
* We need to explore if we can make `aws-lc` the default (at least on
`unix`).
* I want to add optional support for `s2n-tls` to
`aws-smithy-http-client` and reconcile related crypto/tls feature flags
with this in mind.
* Need to figure out how we set the default for `aws-smithy-runtime` and
generated clients to be hyper 1.x and add appropriate new flags, etc.
* Update changelogs, versions, lockfiles, etc.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue
Projects
AWS Rust SDK Public Roadmap
Status: In Progress
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jdisanti @rcoh @Velfi @jmklix @mattklein123