-
Notifications
You must be signed in to change notification settings - Fork 18
feat: Retry requests in more situations #39
Conversation
This flips the retry logic from a flag that says "retry" me, to a flag that says "don't retry me" – and only if the "don't retry me" flag exists do we not retry. A couple of test cases are added for new cases where we now retry, such as socket timeouts and socket hangups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst the change seems fine to me, after doing some testing it doesn't seem to be behaving as I would expect it to. Starting the upload whilst the upload server is not running and starting the upload server within the retry limit causes an "ESOCKETTIMEDOUT" error even if the upload server is running correctly. I don't know whether its request which is the issue or the server.
f5832eb
to
cdfab2e
Compare
Big update to this PR. In investigating the issue mentioned by @sazap10 I ripped out request and replaced it with the Because this library does multipart uploads I had to use the The fix is to call Since I went to the effort of removing request – it's quite a bloaty library – I think it makes sense to leave it out as we simply don't need it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SFTM, works fine with the upload server not being available when starting the upload and becoming available within the retry window.
This flips the retry logic from a flag that says "retry me", to a flag that says "don't retry me" –
and only if the "don't retry me" flag exists do we not retry. A couple of test cases are added for
new cases where we now retry, such as socket timeouts and socket hangups.
Also TIL:
Source: Node.js Process docs
So the env vars that are set in testing which are expected to be integers are now parsed as such.