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

feat(http-client): retry closed connection errors #1336

Merged

Conversation

bpinto
Copy link
Contributor

@bpinto bpinto commented Jan 21, 2022

Changes made

Requests that fail with closed connection errors (ECONNRESET, EPIPE) are now automatically retried.

  • ECONNRESET (Connection reset by peer): A connection was forcibly
    closed by a peer.closed by a peer. This normally results from a loss
    of the connection on the remote socket due to a timeout or reboot.
    Commonly encountered via the http and net modules.

  • EPIPE (Broken pipe): A write on a pipe, socket, or FIFO for which
    there is no process to read the data. Commonly encountered at the net
    and http layers, indicative that the remote side of the stream being
    written to has been closed.

Purpose of changes

Fixes: #1040

From my perspective, handling these errors by retrying is likely the proper approach, and shouldn't necessarily be viewed as a workaround, or masking an underlying issue, because it is expected/unavoidable that these broken connections will come to exist, and there doesn't seem to be an obvious way of asking Node "how long has it been since the last keep-alive probe on this connection" besides writing to the connection and triggering the error.

At the same time, I think we should look into the possibility of making stripe-node handle errors like this by default/more transparently, so that users don't have to configure the retries themselves. That seems to be what Amazon started doing for errors like this for their own SDKs about a month ago (thank you @hisham for linking to that issue, by the way).

See: #1040 (comment)

Depends on #1335

TODO

  • Should this become optional through a config option?
  • Update documentation.
  • Test this change on a lambda environment.

@bpinto bpinto changed the title Retry closed connection errors feat(http-client): retry closed connection errors Jan 21, 2022
@richardm-stripe
Copy link
Contributor

Hello @bpinto,

Thank you so much submitting this - I am quite excited about this. I will try to have a more detailed look at this next week.

@bpinto
Copy link
Contributor Author

bpinto commented Jan 22, 2022

Hey @richardm-stripe 👋

I'm excited about this change as well! Please let me know what you think we could do differently.

P.S.: I've noticed tests are failing and I'll be looking at those soon, I'm flying this Wednesday so I might not have to look at this until later this week.

@bpinto bpinto force-pushed the retry-closed-connection-errors branch from e69f78f to dea0c78 Compare January 28, 2022 01:23
Requests that fail with closed connection errors (ECONNRESET, EPIPE) are
automatically retried.

- `ECONNRESET` (Connection reset by peer): A connection was forcibly
  closed by a peer.closed by a peer. This normally results from a loss
  of the connection on the remote socket due to a timeout or reboot.
  Commonly encountered via the http and net modules.

- `EPIPE` (Broken pipe): A write on a pipe, socket, or FIFO for which
  there is no process to read the data. Commonly encountered at the net
  and http layers, indicative that the remote side of the stream being
  written to has been closed.

Fixes: stripe#1040
@bpinto bpinto force-pushed the retry-closed-connection-errors branch from dea0c78 to 75e10df Compare January 28, 2022 01:24
@richardm-stripe
Copy link
Contributor

Hi @bpinto, just to keep you updated, we are planning on merging this change in the next major as we're a bit hesitant to change the default retry behavior in a minor.

@bpinto
Copy link
Contributor Author

bpinto commented Feb 26, 2022

Totally understandable, thanks for the follow up.

@richardm-stripe richardm-stripe enabled auto-merge (squash) May 9, 2022 19:56
@richardm-stripe richardm-stripe merged commit 47776ef into stripe:master May 9, 2022
pakrym-stripe added a commit that referenced this pull request Jun 8, 2022
* API Updates (#1413)

* Bump version to 8.221.0

* API Updates (#1414)

* Bump version to 8.222.0

* API Updates (#1415)

* feat(http-client): retry closed connection errors (#1336)

* feat(http-client): Retry requests that failed with closed connection

Requests that fail with closed connection errors (ECONNRESET, EPIPE) are
automatically retried.

- `ECONNRESET` (Connection reset by peer): A connection was forcibly
  closed by a peer.closed by a peer. This normally results from a loss
  of the connection on the remote socket due to a timeout or reboot.
  Commonly encountered via the http and net modules.

- `EPIPE` (Broken pipe): A write on a pipe, socket, or FIFO for which
  there is no process to read the data. Commonly encountered at the net
  and http layers, indicative that the remote side of the stream being
  written to has been closed.

Fixes: #1040

* Remove deprecated orders-related events (#1417)

* Bump version to 9.0.0

* API Updates (#1420)

* Codegen for openapi 7789931

* Bump version to 9.1.0

* API Updates (#1422)

* Codegen for openapi 056745c
Co-authored-by: Richard Marmorstein <richardm@stripe.com>
Co-authored-by: Dominic Charley-Roy <dcr@stripe.com>

* Bump version to 9.2.0

* Codegen for openapi v146 (#1430)

* Bump version to 9.3.0

* Codegen for openapi v147 (#1431)

* Bump version to 9.4.0

* docs: Update HttpClient documentation to remove experimental status. (#1432)

* Codegen for openapi v149 (#1434)

* Bump version to 9.5.0

* API Updates (#1439)

* Bump version to 9.6.0

* Update README.md (#1440)

* Codegen for openapi v152 (#1441)

* Add test for cash balance methods. (#1438)

* Bump version to 9.7.0

Co-authored-by: Dominic Charley-Roy <78050200+dcr-stripe@users.noreply.github.com>
Co-authored-by: Dominic Charley-Roy <dcr@stripe.com>
Co-authored-by: Richard Marmorstein <52928443+richardm-stripe@users.noreply.github.com>
Co-authored-by: Bruno Pinto <brunoferreirapinto@gmail.com>
Co-authored-by: Richard Marmorstein <richardm@stripe.com>
Co-authored-by: Kamil Pajdzik <99290280+kamil-stripe@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent Error: write EPIPE when running stripe client in AWS Lambda
2 participants