Commit bfe50e2
authored
Fixing test in
**Note:** This PR will need to be released and have at least one daily
release run to get the changes into the `aws-sdk-s3` crate before the
`feature/http-1.x` branch can be successfully merged to main.
## 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 -->
When merging `main` back to the `feature/http-1.x` branch in
#4422 we started to see a
new failure in the [semver hazards
test](https://github.com/smithy-lang/smithy-rs/actions/runs/19772608981/job/56659623913).
```
failures:
---- aws_chunked::tests::test_aws_chunked_body_is_retryable stdout ----
thread 'aws_chunked::tests::test_aws_chunked_body_is_retryable' panicked at sdk/s3/src/aws_chunked.rs:195:47:
called `Result::unwrap()` on an `Err` value: "Trailers polled while body still has data"
```
The semver hazards test works by checking out the published versions of
the SDK crates in the `aws-sdk-rust` repo, patching those crates to use
the PRed versions of the runtime crates, and running the tests in the
SDK crates with those patches in place.
The currently published version of `test_aws_chunked_body_is_retryable`
[creates a temp
file](https://github.com/awslabs/aws-sdk-rust/blob/d8ccbf91973613be3c4c618fef7f452b347749af/sdk/s3/src/aws_chunked.rs#L168-L171)
with size `10000 x line.len()`. The `AwsChunkedBodyOptions` are created
with `::default()` which sets `stream_length = 0` . That means we hit
[this
check](https://github.com/smithy-lang/smithy-rs/blob/d38979ea7f6a3e2f12f5430689f2502a8c00ad38/aws/rust-runtime/aws-runtime/src/content_encoding.rs#L188-L192)
in the `http_body_04x::Body` impl for `AwsChunkedBody` which sets the
state to `AwsChunkedBodyState::WritingTrailers` without ever polling the
inner body. The next time we poll we call `poll_trailers` on the [inner
body](https://github.com/smithy-lang/smithy-rs/blob/d38979ea7f6a3e2f12f5430689f2502a8c00ad38/aws/rust-runtime/aws-runtime/src/content_encoding.rs#L228-L229)
it ends up at the ["Trailers polled while body still has data"
error](https://github.com/smithy-lang/smithy-rs/blob/d38979ea7f6a3e2f12f5430689f2502a8c00ad38/rust-runtime/aws-smithy-types/src/body.rs#L301-L302).
This error is correct, we are polling trailers while the body still has
data.
It turns out Yuki already did a fix for this in his [merge from `main`
to the feature
branch](#4391) by setting
the `stream_length` and changing the [asserts to properly look for the
body
data](https://github.com/smithy-lang/smithy-rs/blob/d38979ea7f6a3e2f12f5430689f2502a8c00ad38/aws/rust-runtime/aws-inlineable/src/aws_chunked.rs#L204-L231).
That change is in an inlineable and still in the feature branch, so the
published version of the S3 crate used by the semver hazards check
doesn't have the fixed test.
The minimum fix here is to port the changes for the tests in
`aws_chunked` to main since the current version of those tests is kind
of wrong anyway, not polling through the body data at all. We got
(un)lucky this showed up in the feature branch because the http-1x
`SdkBody` checks for body data when polling trailers since they can't be
polled separately.
But all of this debugging brings us to the underlying bug, when the
inner body of an `SdkBody` is `BoxBody::HttpBody04` our implementation
will happily let you poll trailers while the body still has data. As far
as I am aware there is no use case that would make this a reasonable
thing to do. To fix the actual bug I see two possible solutions:
* We could fix the underlying bug by updating the `BoxBody::HttpBody04`
branch in `SdkBody`'s `poll_next_trailer` method to return an error by
calling `poll_data` on the inner body in `poll_next_trailers` and
failing if it returns anything other than `Poll::Ready(none)`.
Unfortunately it seems possible that this could cause tricky runtime
breakages for existing users, so probably isn't a tenable option.
* Update the `BoxBody::HttpBody1` branch in `SdkBody`'s
`poll_next_trailer` method to act more like the `BoxBody::HttpBody04`
branch. In this case if it encountered a data frame rather than a
trailer frame it would keep polling the frames until the body data is
exhausted (either buffering or just throwing away the data frames) and
we get a trailer frame or run out of frames. This feels incorrect as
polling the trailers before the body is exhausted shouldn't ever be
correct, and polling the trailer in `HttpBody04` doesn't actually throw
away the body bytes, they could still be polled later.
## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
I used `runtime-versioner patch-runtime` locally to patch the
`feature/http-1.x` branch versions of the runtime crates into a local
checkout of the`aws-sdk-s3` crate. This reproduced the failure locally.
Manually updating the inlineable `aws_chunked.rs` in the S3 crate with
the changes from this PR allowed the tests to pass.
----
_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._aws_chunked inlineable (#4430)1 parent a54774e commit bfe50e2
1 file changed
+8
-2
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
184 | 184 | | |
185 | 185 | | |
186 | 186 | | |
| 187 | + | |
187 | 188 | | |
188 | 189 | | |
189 | 190 | | |
| |||
200 | 201 | | |
201 | 202 | | |
202 | 203 | | |
203 | | - | |
| 204 | + | |
204 | 205 | | |
205 | 206 | | |
206 | 207 | | |
| |||
220 | 221 | | |
221 | 222 | | |
222 | 223 | | |
223 | | - | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
224 | 230 | | |
225 | 231 | | |
226 | 232 | | |
| |||
0 commit comments