disable stalled stream protection on empty bodies and after read complete #3644

aajtodd · 2024-05-14T21:31:04Z

Motivation and Context

Disable StalledStreamProtection for S3 CopyObject awslabs/aws-sdk-rust#1141
Unable to perform multipart upload after upgrade: current throughput: 0 B/s is below minimum: 1 B/s awslabs/aws-sdk-rust#1146
Upgrade to aws-sdk-sqs="1.23.0" causing timeouts awslabs/aws-sdk-rust#1148

Description

Disables stalled stream upload protection for requests with an empty/zero length body.
Disables stalled stream upload throughput checking once the request body has been read and handed off to the HTTP layer.

Testing

Additional integration tests added covering empty bodies and completed uploads.

Tested SQS issue against latest runtime and can see it works now. The S3 CopyObject issue is related to downloads and will need a different solution.

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…been handed off to http layer

github-actions · 2024-05-14T21:48:57Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-05-15T13:33:15Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-05-15T17:27:07Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-05-16T13:21:24Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

add tests for is_end_stream to ByteStream add tests for size_hint to ByteStream fix http_body_consumes_data test

github-actions · 2024-05-16T20:01:23Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

aajtodd · 2024-05-16T20:56:48Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput/options.rs

@@ -87,7 +87,7 @@ pub struct MinimumThroughputBodyOptionsBuilder {
    grace_period: Option<Duration>,
 }

-const DEFAULT_GRACE_PERIOD: Duration = Duration::from_secs(0);
+const DEFAULT_GRACE_PERIOD: Duration = Duration::from_secs(20);


correctness: Is this the right one?

I also see

smithy-rs/rust-runtime/aws-smithy-runtime-api/src/client/stalled_stream_protection.rs

Line 16 in 22a0e30

const DEFAULT_GRACE_PERIOD: Duration = Duration::from_secs(5);

Whats the difference between these two? 🤔 , confusing that we have two constants for this..

ah nuts, no wonder. Let's delete one.

github-actions · 2024-05-17T17:55:52Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

fix is_end_stream for 0 length PathBody that hasn't been polled

github-actions · 2024-05-17T18:38:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

landonxjames

Based on our discussions the other day I think this covers everything besides the customization for S3 CopyObject that we agreed to defer to another PR. One comment on a method name I found confusing but otherwise looks good!

landonxjames · 2024-05-17T18:44:56Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput/throughput.rs

+    ///
+    /// After marking a stream complete [report](#method.report) will forever more return
+    /// [ThroughputReport::Complete]
+    pub(super) fn complete(&mut self) -> bool {


Could we name this mark_complete or something to indicate that it has a side effect? When I saw this method used in UploadThroughput I assumed it was just checking the completeness, not setting it. And with that the complete method in UploadThroughput should probably be renamed as well.

heh it was originally named that, yeah I think thats fine

Velfi · 2024-05-17T19:08:57Z

Since Landon mentioned it, here's the "No SSP for CopyObject" PR #3649

aajtodd · 2024-05-17T19:02:23Z

rust-runtime/aws-smithy-runtime-api/src/client/stalled_stream_protection.rs

+///
+/// When a stream stalls for longer than this grace period, the stream will
+/// return an error.
+pub const DEFAULT_GRACE_PERIOD: Duration = Duration::from_secs(20);


b/c there's an SSP option struct in aws-smithy-runtime that also has a default and I wanted a single source of truth.

aajtodd · 2024-05-17T19:05:25Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput/options.rs

 use std::time::Duration;

-/// A collection of options for configuring a [`MinimumThroughputBody`](super::MinimumThroughputBody).
+/// A collection of options for configuring a [`MinimumThroughputBody`](super::MinimumThroughputDownloadBody).


this was correct as it was I think, it applies to MinimumThroughputBody not just MinimumThroughputDownloadBody

I did it to avoid the deprecation message:

/// Use [`MinimumThroughputDownloadBody`] instead. #[deprecated(note = "Renamed to MinimumThroughputDownloadBody since it doesn't work for uploads")] pub type MinimumThroughputBody<B> = MinimumThroughputDownloadBody<B>;

aajtodd · 2024-05-17T19:06:14Z

rust-runtime/aws-smithy-runtime/src/client/http/body/minimum_throughput/throughput.rs

+    ///
+    /// After marking a stream complete [report](#method.report) will forever more return
+    /// [ThroughputReport::Complete]
+    pub(super) fn complete(&mut self) -> bool {


heh it was originally named that, yeah I think thats fine

aajtodd · 2024-05-17T19:09:29Z

rust-runtime/aws-smithy-types/src/byte_stream/bytestream_util.rs

@@ -230,15 +233,18 @@ impl FsBuilder {
 enum State {
    Unloaded(PathBuf),
    Loading(Pin<Box<dyn Future<Output = io::Result<File>> + Send + Sync + 'static>>),
-    Loaded(ReaderStream<io::Take<File>>),
+    Loaded {


question: I assume this is the case but this gets reset correctly on a retry right?

Yeah. The PathBody is reconstructed from scratch each time.

ysaito1001 · 2024-05-17T19:21:05Z

CHANGELOG.next.toml

@@ -36,6 +36,18 @@ references = ["aws-sdk-rust#1079"]
 meta = { "breaking" = false, "bug" = true, "tada" = false }
 author = "rcoh"

+[[aws-sdk-rust]]
+message = "Fixes stalled upload stream protection to not apply to empty request bodies and to stop checking for violations once the request body has been read."
+references = ["aws-sdk-rust#1141", "aws-sdk-rust#1146", "aws-sdk-rust#1148"]


Will aws-sdk-rust#1141 (for CopyObject) and aws-sdk-rust#1146 (for MultipartUpload) be addressed by this PR?

CopyObject is going to be resolved in a separate PR that I linked in an earlier post. MPU should be fixed by this PR.

Gotcha, maybe we should remove aws-sdk-rust#1141 from references then?

rust-runtime/aws-smithy-runtime/tests/stalled_stream_upload.rs

Co-authored-by: ysaito1001 <awsaito@amazon.com>

rust-runtime/aws-smithy-runtime/Cargo.toml

CHANGELOG.next.toml

rust-runtime/aws-smithy-runtime/tests/stalled_stream_upload.rs

ysaito1001

nit

rust-runtime/aws-smithy-types/src/byte_stream.rs

Co-authored-by: ysaito1001 <awsaito@amazon.com>

github-actions · 2024-05-17T20:08:09Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

disable stalled stream protection on empty bodies and after data has …

f84e5fb

…been handed off to http layer

aajtodd added 3 commits May 15, 2024 08:13

changelog, bump verison, and fix warnings

07d5e51

fix download test

a9d19d4

fix multiple version bumps

c30e54d

possible fix for early end of polling

bc4b1db

add additional tests

22a0e30

set SSP grace period to 20s

fccc930

add tests for is_end_stream to ByteStream add tests for size_hint to ByteStream fix http_body_consumes_data test

aajtodd commented May 16, 2024

View reviewed changes

Velfi added 2 commits May 17, 2024 12:37

fix is_end_stream check for FsBuilder byte streams

711fe26

Merge branch 'main' into atodd/fix-stalled-streams

e08df6a

unify DEFAULT_GRACE_PERIOD consts

028af04

fix is_end_stream for 0 length PathBody that hasn't been polled

Velfi marked this pull request as ready for review May 17, 2024 18:21

Velfi requested review from a team as code owners May 17, 2024 18:21

landonxjames approved these changes May 17, 2024

View reviewed changes

remove unused dependency

e2e3824

aajtodd commented May 17, 2024

View reviewed changes

respond to PR comments

1b282f2

ysaito1001 reviewed May 17, 2024

View reviewed changes

rust-runtime/aws-smithy-runtime/tests/stalled_stream_upload.rs Outdated Show resolved Hide resolved

Velfi and others added 2 commits May 17, 2024 12:30

Update rust-runtime/aws-smithy-runtime/tests/stalled_stream_upload.rs

287dd10

Co-authored-by: ysaito1001 <awsaito@amazon.com>

rename method mark_complete

a07103b

ysaito1001 approved these changes May 17, 2024

View reviewed changes

rust-runtime/aws-smithy-runtime/Cargo.toml Outdated Show resolved Hide resolved

CHANGELOG.next.toml Outdated Show resolved Hide resolved

rust-runtime/aws-smithy-runtime/tests/stalled_stream_upload.rs Outdated Show resolved Hide resolved

ysaito1001 reviewed May 17, 2024

View reviewed changes

rust-runtime/aws-smithy-types/src/byte_stream.rs Outdated Show resolved Hide resolved

rust-runtime/aws-smithy-types/src/byte_stream.rs Outdated Show resolved Hide resolved

Velfi and others added 2 commits May 17, 2024 12:47

Apply suggestions from code review

3b27db8

Co-authored-by: ysaito1001 <awsaito@amazon.com>

respond to PR comments

cd893fc

Velfi added this pull request to the merge queue May 17, 2024

Merged via the queue into main with commit f0ddc66 May 17, 2024
44 checks passed

Velfi deleted the atodd/fix-stalled-streams branch May 17, 2024 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disable stalled stream protection on empty bodies and after read complete #3644

disable stalled stream protection on empty bodies and after read complete #3644

aajtodd commented May 14, 2024 •

edited by Velfi

Loading

github-actions bot commented May 14, 2024

github-actions bot commented May 15, 2024

github-actions bot commented May 15, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

aajtodd May 16, 2024

aajtodd May 17, 2024

Velfi May 17, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

landonxjames left a comment

landonxjames May 17, 2024

aajtodd May 17, 2024

Velfi commented May 17, 2024

aajtodd May 17, 2024

Velfi May 17, 2024

aajtodd May 17, 2024

Velfi May 17, 2024

aajtodd May 17, 2024

aajtodd May 17, 2024

Velfi May 17, 2024

ysaito1001 May 17, 2024

Velfi May 17, 2024

ysaito1001 May 17, 2024

ysaito1001 left a comment

github-actions bot commented May 17, 2024

disable stalled stream protection on empty bodies and after read complete #3644

disable stalled stream protection on empty bodies and after read complete #3644

Conversation

aajtodd commented May 14, 2024 • edited by Velfi Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented May 14, 2024

github-actions bot commented May 15, 2024

github-actions bot commented May 15, 2024

github-actions bot commented May 16, 2024

github-actions bot commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

landonxjames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Velfi commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysaito1001 left a comment

Choose a reason for hiding this comment

github-actions bot commented May 17, 2024

aajtodd commented May 14, 2024 •

edited by Velfi

Loading