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

maxNetworkRetries setting does not work #1058

Closed
rilian opened this issue Oct 28, 2020 · 12 comments · Fixed by #1104
Closed

maxNetworkRetries setting does not work #1058

rilian opened this issue Oct 28, 2020 · 12 comments · Fixed by #1104
Assignees

Comments

@rilian
Copy link

rilian commented Oct 28, 2020

environment: node v10.x on AWS Lambda
package version "stripe": "^8.84.0"

we have the following code in our checkout lambda function, as suggested in official docs https://www.npmjs.com/package/stripe#network-retries

const stripe = require('stripe')(process.env.STRIPE_API_KEY, {
  maxNetworkRetries: 3,
  timeout: 4000,
  telemetry: false,
})

module.exports = async (req, res) => {
  // ...
  await stripe.invoices.pay(invoice.id)
  // ...
}

in one of executions it thrown exception

Request aborted due to timeout being reached (4000ms)

we expected request would be retried at least 3 times, but it did not, cause Lambda function duration was ~5000ms

_scr_ 2020-10-28 at 14 34 44

@ctrudeau-stripe
Copy link
Contributor

Hello @rilian, I think stripe node works as you expect with respect to request timeouts but not socket timeouts. Presently the timeout applies only to a request after a connection has been successfully established. Under the hood we set req.setTimeout and use the value passed into the timeout in the config object. It can also happen that your host OS can not successfully establish a connection in the first place and the timeout here is the socket timeout that is specified by your operating system. Can you give us more details around the error happening where it's printing 'Could not pay Invoice Request aborted due to timeout being reached`? Thank you!

@rilian
Copy link
Author

rilian commented Dec 11, 2020

hi @ctrudeau-stripe do you have any updates regarding fixing this issue ? we got it again, in a slightly different scenario

in new case, our lambda service was only requesting stripe.subscriptions.list, and our stripe ("stripe": "^8.119.0") configuration is

const stripe = require('stripe')(process.env.STRIPE_API_KEY, {
  maxNetworkRetries: 2,
  timeout: 10000,
  telemetry: false,
})

and Lambda execution duration was definitely less than time of 2 retries - it ended after 10 sec meaning there was NO retry at all

REPORT RequestId: 772f57a3-d020-4788-aba4-70d1d98cdf85	Duration: 10183.32 ms	Billed Duration: 10184 ms	Memory Size: 1024 MB	Max Memory Used: 180 MB	

i would like to add that this is a test stripe key, and we did many requests in a loop in parallel, but in any case, even if request was rate limited it had to receive a 429 HTTP status instead of timeout that was not retried

the exception that we got is

Error trace: Error: Request aborted due to timeout being reached (10000ms)
    at ClientRequest.<anonymous> (/var/task/node_modules/stripe/lib/StripeResource.js:106:9)
    at Object.onceWrapper (events.js:286:20)
    at ClientRequest.emit (events.js:198:13)
    at ClientRequest.EventEmitter.emit (domain.js:448:20)
    at TLSSocket.emitRequestTimeout (_http_client.js:673:40)
    at Object.onceWrapper (events.js:286:20)
    at TLSSocket.emit (events.js:198:13)
    at TLSSocket.EventEmitter.emit (domain.js:448:20)
    at TLSSocket.Socket._onTimeout (net.js:443:8)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)

@ctrudeau-stripe
Copy link
Contributor

ctrudeau-stripe commented Dec 12, 2020

Hello @rilian, are you able to verify you were able to establish a socket? If it was not able to establish a socket the maxNetworkRetries parameter will not work. I'm wondering from your stack trace if the socket was able to establish or not.

@rilian
Copy link
Author

rilian commented Dec 12, 2020

@ctrudeau-stripe how would you recommend verifying that ?

other 1000s requests today were fine (did not timeout in 10sec)..

@rilian
Copy link
Author

rilian commented Dec 21, 2020

@ctrudeau-stripe please review ^^

@remi-stripe
Copy link
Contributor

@rilian Sorry for the delays! I tagged internally and we'll review this after the holidays!

@ctrudeau-stripe
Copy link
Contributor

Hello @rilian sorry for the delays. We've looked into this more and we were not retrying a request that had timed out. We believe we should be respecting the retries for those as you expect. The above PR #1104 will be merged in the next major release.

@rilian
Copy link
Author

rilian commented Jan 5, 2021

@ctrudeau-stripe thank you, i'll subscribe to that PR and will watch for release

@rilian
Copy link
Author

rilian commented Jan 15, 2021

hi @ctrudeau-stripe . i watched for stripe-node releases and this was not merged in last 2 releases. Can you please clarify what you mean by "next major version". Do you plan to fix it in v9 ? ... thank you! It's very important fix for stability of payments for all customers

@richardm-stripe
Copy link
Contributor

Hi @rilian, thanks for following up. After some internal discussion, we've decided to consider this change a non-breaking bugfix, since the API is preserved, and this has been released in v8.131.1.

@rilian
Copy link
Author

rilian commented Jan 15, 2021

@richardm-stripe fantastic, thank you!

@rilian
Copy link
Author

rilian commented Feb 22, 2021

@richardm-stripe
today we again got Error: Error: ESOCKETTIMEDOUT in the request to create InvoiceItem

using "stripe": "^8.132.0",

request timed out after 10 sec, with this configuration:

const stripe = require('stripe')(process.env.STRIPE_API_KEY, {
  maxNetworkRetries: 2,
  timeout: 10000,
  telemetry: false,
})

and was not retried.

with configuration above we expected it to be retried at least 2 times

timing report from Lambda:

REPORT RequestId: 1d25bfe9-1661-4351-a741-cb462da76199	Duration: 10697.11 ms	Billed Duration: 10698 ms	

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 a pull request may close this issue.

4 participants