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.

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

Retry additional classes of H2 errors #2971

Merged
merged 2 commits into from
Sep 12, 2023
Merged

Retry additional classes of H2 errors #2971

merged 2 commits into from
Sep 12, 2023

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Sep 5, 2023

Draft pull request pending merge of #2970

Motivation and Context

Description

This PR adds two additional classes of retries tested:

  1. GO_AWAY: Retry after HTTP2 GOAWAY errors awslabs/aws-sdk-rust#738
  2. REFUSED_STREAM: DispatchFailure: HTTP/2 REFUSED_STREAM should be retried awslabs/aws-sdk-rust#858

I tested 1 by using the example helpfully provided. The fix for 2 is untested and difficult to reproduce but since my fix for 1 worked, I'm confident that we're detecting the correct error class here.

Testing

I used the example provided by the customer and validated the H2 GO_AWAY fix.

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.

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

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.

@rcoh rcoh marked this pull request as ready for review September 7, 2023 18:34
@rcoh rcoh requested a review from a team as a code owner September 7, 2023 18:34
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2970 has been merged, and this PR looks good.

Comment on lines 212 to 214
if err.is_closed() {
return ConnectorError::io(err.into());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this already covered by the first guard condition at line 205?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good catch

This PR adds two additional classes of retries tested:
1. GO_AWAY: awslabs/aws-sdk-rust#738
2. REFUSED_STREAM: awslabs/aws-sdk-rust#858

I tested 1 by using the example helpfully provided. The fix for 2 is untested and difficult to reproduce but since my fix for 1 worked, I'm confident that we're detecting the correct error class here.
@github-actions
Copy link

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.

Merged via the queue into main with commit 7bf8837 Sep 12, 2023
41 checks passed
@rcoh rcoh deleted the h2-retries branch September 12, 2023 18:30
@ferologics
Copy link

has this been shipped yet? we'd like to solve for awslabs/aws-sdk-rust#738

@rcoh
Copy link
Collaborator Author

rcoh commented Nov 23, 2023 via email

@ferologics
Copy link

Hey @rcoh thanks, I'm having a hard time figuring out when this was released. I don't see any entry referencing this PR in your changelog. We are using version 0.57.1 in our project and seeing the issue. I can just update to latest and cross my fingers here, but maybe you can point me to the right place?

image

@rcoh
Copy link
Collaborator Author

rcoh commented Nov 24, 2023

it was released, but then a bad merge appears to have reverted these fixes. I'm re-fixing it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants