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

Usage of http.DefaultClient has potential to back-up all network calls #3357

Closed
3 tasks done
joshprzybyszewski-wf opened this issue Jun 8, 2020 · 3 comments
Closed
3 tasks done
Labels
feature-request A feature should be added or improved. performance

Comments

@joshprzybyszewski-wf
Copy link

joshprzybyszewski-wf commented Jun 8, 2020

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
Sometimes a golang application deployed to AWS may unexplainably halt (almost all) network traffic. According to golang/go#33006 (comment), it seems possible for http requests to block for a long time. Here aws-sdk-go uses the http.DefaultClient and I see no other calls of WithHTTPClient that would override this. Could we choose an http client that is not http.DefaultClient to use in the default config defined here?

See this blog post for additional information.

Version of AWS SDK for Go?
v1.26.3

  • get SDK version by printing the output of aws.SDKVersion in your code after importing "github.com/aws/aws-sdk-go/aws"

Version of Go (go version)?
go 1.14.2

To Reproduce (observed behavior)
This is bug is difficult to reliably reproduce and seems to be a problem with the http package, not aws-sdk-go directly. See the steps to repro in the linked golang issue (golang/go#33006)

Expected behavior
aws should never block on http messaging

Additional context

@joshprzybyszewski-wf joshprzybyszewski-wf added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 8, 2020
@joshprzybyszewski-wf joshprzybyszewski-wf changed the title Usage of http.DefaultClient has potential to backend all aws calls Usage of http.DefaultClient has potential to back-up all aws calls Jun 8, 2020
@joshprzybyszewski-wf joshprzybyszewski-wf changed the title Usage of http.DefaultClient has potential to back-up all aws calls Usage of http.DefaultClient has potential to back-up all network calls Jun 8, 2020
@jasdel jasdel added performance feature-request A feature should be added or improved. and removed bug This issue is a bug. labels Jun 15, 2020
@jasdel
Copy link
Contributor

jasdel commented Jun 15, 2020

Thanks for reporting this issue @joshprzybyszewski-wf and informing us about the issue with the Go HTTP client. From reading this, it sounds like using the Default HTTP client exacerbates the issue since it can be potentially shared by additional logic within the application outside of the SDK.

A partial mitigation would be to ensure that when using the SDK, that the application always passes in a HTTP client to the API client or Session when creating one. With the SDK this requires passing in a custom HTTP client value for each session or API client created. The SDK's Developer Guide on Creating a custom HTTP client has some details on how to do this with the SDK.

The V2 SDK in developer preview does change its behavior significantly in regard to the default HTTP client by defaulting to an internal HTTP client that will ensure each API client created will have a unique instance of the underlying HTTP client. This implementations cannot be direclty back ported to the v1 SDK since the v2 SDK switched to an interface for the HTTP client, vs the v1 SDK's concrete type.

One workaround for this in the short term would be to create an "clean" HTTP client per SDK API client created. The V2 SDK's implementation of the BuildableHTTPClient is a utility you could copy in order to make it easier to configure multiple clients at the same time without sharing the HTTP client instances across the SDK. I don't recommend taking a dependency on the V2 SDK direclty at the moment, as its codebase is in significant flux and breaking changes are expected.

@jasdel jasdel removed the needs-triage This issue or PR still needs to be triaged. label Jun 15, 2020
@skmcgrail
Copy link
Member

The V2 SDK GA'd on 01/19 which by default does not use the HTTP default client. We are are not planning to change the default HTTP client in the V1 SDK, as to do so would be a breaking change in behavior. Our suggestion is for users to start exploring migrating their applications to the V2 SDK.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. performance
Projects
None yet
Development

No branches or pull requests

3 participants