Adds retry-on-error for errors 408/500 #6413
Conversation
arcanis
force-pushed
the
500-retries
branch
from
3e51e3e
to
10ae0ba
Compare
src/util/request-manager.js
Outdated
| if (body && typeof body.error === 'string') { | ||
| reject(new Error(body.error)); | ||
| return; | ||
| } | ||
|
|
||
| if (res.statusCode === 403) { | ||
| if ([403, 408].indexOf() !== -1 || res.statusCode >= 500) { |
There was a problem hiding this comment.
Isn't res.statusCode mising in the indexOf()?
There was a problem hiding this comment.
Yep, it totally does ... Flow should have warned me 😞
|
No worry! I updated the code to take all |
|
Glad to see this merged! Thanks for your work here @arcanis 🙏 |
Now that 1.11.0 is released with yarnpkg/yarn#6413 merged, Heroku users should see fewer failed builds due to 500 responses from the registry.
Now that 1.11.0 is released with yarnpkg/yarn#6413 merged, Heroku users should see fewer failed builds due to 500 responses from the registry.
|
Hi @arcanis, this retry mechanism is very welcome thank you! However the retry message is I think a little misleading and IMO very annoying 😄 Given this mechanism is mostly to address congenital unreliability of the npm registries, is it fair that the message suggests it is the user's local network that is the problem? Users are like, 'what do you mean my network has a problem, I have a gazillion-bit connection with zero drops!'. And they are looking for faults and problems in their network and for bugs in yarn, when we know neither is likely the cause. Could you consider revising or crafting a more appropriate message for this type of retry? Also, given this retry requirement is congenital (I get multiple of this message with every |
|
Hey @whereisaaron - the error message should be more explicit than what you quote. Cf here: https://github.com/yarnpkg/yarn/pull/6413/files#diff-49d57266c25cd3c6ddab1f59de18b36bR24 Note that this PR is only part of the 1.11 and 1.12 branches, which are yet in prerelease. |
|
Thanks @arcanis that message does look much better - I had thought we were seeing the message from this line of your patch - which was the one I was quoting. Great - so hopefully when we get 1.11 we'll see the current messages replaced with your new, more accurate message. 🎉 |
Fixes tests from yarnpkg#6413
Summary
Supersedes #6400, #6383
This PR adds a retry mechanism for when the npm registry returns 408 / 500 errors.
Test plan
Added a test. Hopefully our own CI will become more stable as well.