Retries are abandoned if one of them results in an errorous response

I believe this is related to #113

If a retry gets an errorous response, it looks like it just gives up entirely without continuing even if there are more attempts available.

To reproduce this, I've made a small setup for it: https://github.com/mitom/node-downloader-helper-issue The output looks like

❯ node client/downloader.js
State:  STARTED
State:  FAILED
Something happened Error: Response status was 502
    at ClientRequest.<anonymous> (/Users/mitom/projects/file-server-test/node_modules/node-downloader-helper/dist/index.js:1:9499)
    at Object.onceWrapper (node:events:634:26)
    at ClientRequest.emit (node:events:519:28)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (node:_http_client:693:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17)
    at Socket.socketOnData (node:_http_client:535:22)
    at Socket.emit (node:events:519:28)
    at addChunk (node:internal/streams/readable:559:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
    at Readable.push (node:internal/streams/readable:390:5) {
  status: 502,
  body: ''
}
failed to start download, retrying attempt 1 Error: Response status was 502
    at ClientRequest.<anonymous> (/Users/mitom/projects/file-server-test/node_modules/node-downloader-helper/dist/index.js:1:9499)
    at Object.onceWrapper (node:events:634:26)
    at ClientRequest.emit (node:events:519:28)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (node:_http_client:693:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17)
    at Socket.socketOnData (node:_http_client:535:22)
    at Socket.emit (node:events:519:28)
    at addChunk (node:internal/streams/readable:559:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
    at Readable.push (node:internal/streams/readable:390:5) {
  status: 502,
  body: ''
}
State:  STARTED
Download Begins:  { name: 'test (12).file', total: 10485760 }
State:  DOWNLOADING
first progress
download started
download is running
0/s - 3.6% [0.4/10]
0.6/s - 6.2% [0.6/10]
State:  RETRY
{ RetryAttempt: '1/50', StartsOn: '2 secs', Reason: 'aborted' }
State:  RESUMED
This URL doesn't support resume, it will start from the beginning
State:  FAILED
Something happened Error: Response status was 502
    at ClientRequest.<anonymous> (/Users/mitom/projects/file-server-test/node_modules/node-downloader-helper/dist/index.js:1:9499)
    at Object.onceWrapper (node:events:634:26)
    at ClientRequest.emit (node:events:519:28)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (node:_http_client:693:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17)
    at Socket.socketOnData (node:_http_client:535:22)
    at Socket.emit (node:events:519:28)
    at addChunk (node:internal/streams/readable:559:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
    at Readable.push (node:internal/streams/readable:390:5) {
  status: 502,
  body: ''
}
download finished

The thing to note is even though we're on re-try 1 of 50, the error ends the entire thing.

The example downloader is largely taken from the example in this repo, but with 1 somewhat minor change - the function resolves the promise once the download is in progress and then we have a 2nd await for when it's completed. This matches my usecase as I want to be able to interact with the file (read parts of it) while the download is running, but the issue exists without anything else accessing the file (i.e. as above in the reproduction).

I've tried to dig around in the code for why this is happening, but not really managed to find it. What I could tell is that while the error handler is set up in https://github.com/hgouveia/node-downloader-helper/blob/master/src/index.js#L445, it looks like in the case of the request for resume failing, those never actually get executed.

Any help would be appreciated, thanks!

@hgouveia I kind of know why this is happening now but I'm not really sure what would be the right path to fix it.

The issue is that these failures come up as a close (https://nodejs.org/api/http.html#event-close) event rather than an error event in http.ClientRequest. The close event seems to indicate that is may be due to an error so some extra checking is required around it. So we need another event listener in https://github.com/hgouveia/node-downloader-helper/blob/master/src/index.js#L445

Just to test, I've simply added the same onError listener as the error event has on the close event too, which yields another issue. These requests get marked as state FAILED in the library, which seems to exclude them from being re-tried. I don't really understand why this is the case (why are they being marked as failed while we have retries left) but I'm assuming it's because the decision of what to retry is not a user-provided callback so you simply decided on some scenarios for it. Please do correct me on that one, it's just an assumption.

So all in all adding the onError listener on close and removing the condition for checking FAILED in https://github.com/hgouveia/node-downloader-helper/blob/master/src/index.js#L694 makes the retries happen, but I'm pretty sure it breaks other things (e.g. as is, it'd consider it an error if the download finishes successfully and re-try it, the close event doesn't tell us why it was closed so it needs some other way to check if it finished downloading or not, which I'm not sure how to do tbh).

One path that I see as an option here is to collect the current logic of "can this be retried" into a function, then provide an option to pass in a callback with the same arguments to override it so the user can decide whether a request is retryable or not.

The other option I guess is to only set state to FAILED if we have ran out of retries - although this would change the current behaviour in a way that may not be what's desired.

There are probably other paths, but overall I don't think I understand what's the expectation well enough to confidently fix this. Do you have any ideas on how to fix the problem?

hgouveia / node-downloader-helper

Retries are abandoned if one of them results in an errorous response #115