Error event is emitted before every try even if download finished successfully

[Envek]

Hey, thank you for this package, it helps.

However, I found some confusing behavior that I believe is a bug. error event is emitted after every unsuccessful try even if download was successful.

In my understanding of EventEmitter errors, error event is (usually) a fatal one: it can't be recovered and you need to restart everything from scratch.

For example:

const { DownloaderHelper } = require("node-downloader-helper")

(async () => {
  const dl = new DownloaderHelper(
    "https://example.com/",
    ".", {
      fileName: "anything",
      retry: { maxRetries: 3, delay: 50 },
    }
  );
  dl.on("retry", console.debug);
  dl.on("error", console.error);
  dl.on("end", console.info);
  await dl.start();
})()

Suppose that network error occurs for first two times, but on third try file downloads successfully.

Expected behavior:

error event isn't fired if download was successful after all.

DEBUG: 1 { maxRetries: 3, delay: 50 }
DEBUG: 2 { maxRetries: 3, delay: 50 }
INFO:  {  fileName: 'anything', … , downloadedSize: 1256 }

Actual behavior:

error event is emitted before every try.

ERROR: Error: ECONNRESET
DEBUG: 1 { maxRetries: 3, delay: 50 }
ERROR: Error: ECONNRESET
DEBUG: 2 { maxRetries: 3, delay: 50 }
INFO:  {  fileName: 'anything', … , downloadedSize: 1256 }

[Chaphasilor]

Hey, as far as I understand it the error event doesn't have to be a fatal, non-recoverable error.
However, if you want to catch fatal errors, do it by catching the promise of the dl.start() method!

this.dl.start().catch(err => {

  this.status = `failed`;
  this.emit(`failed`, err);
  return;

});

Hope this helps :)

[Envek]

No, error events by default are fatal, see Events: Error events in the Node.js docs:

If an EventEmitter does not have at least one listener registered for the 'error' event, and an 'error' event is emitted, the error is thrown, a stack trace is printed, and the Node.js process exits.

So, if I run following code in node --eval (not in REPL, it is important):

const { DownloaderHelper } = require("node-downloader-helper");
(async () => {
  const dl = new DownloaderHelper(
    "https://nonexist.example.com",
    ".",
    { fileName: "anything", retry: { maxRetries: 3, delay: 50 } },
  );
  dl.on("retry", console.debug);
  dl.on("end", console.info);
  try {
    await dl.start()
  } catch (e) {
    throw new Error(`I GIVE UP! ${e.message}`)
  }
})()                   

Expected result:

3 retries are performed, custom error is thrown:

1 { maxRetries: 3, delay: 50 }
2 { maxRetries: 3, delay: 50 }
3 { maxRetries: 3, delay: 50 }
(node:23020) UnhandledPromiseRejectionWarning: Error: I GIVE UP! getaddrinfo ENOTFOUND nonexist.example.com

Actual result:

No retries performed, original error is thrown:

events.js:292
      throw er; // Unhandled 'error' event
      ^

Error: getaddrinfo ENOTFOUND nonexist.example.com
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26)
Emitted 'error' event on b instance at:
    at ClientRequest.<anonymous> (/home/envek/evl.ms/2uinc/ext-admissions-workflow/node_modules/node-downloader-helper/dist/index.js:1:10025)

Workaround:

Always add some dummy error handler:

dl.on("error", () => {});

[hgouveia]

Hello @Envek , i just added a fix for this, i wonder if you could try it by install it like this npm install --save hgouveia/node-downloader-helper#dev , and check if works as expected now before merge with master, thanks!

[Envek]

Now it performs 3 retries as expected, but then it emits error event 4 times (looks like one for every retry and then one “final”), while I would expect only one error emit after all retries.

Now it works almost as expected, thank you!

Here is runnable example and its output

You can paste it right into bash or zsh shell: ```sh xargs -0 <<'JS' node --eval const { DownloaderHelper } = require("node-downloader-helper"); (async () => { const dl = new DownloaderHelper( "https://nonexist.example.com", ".", { fileName: "anything", retry: { maxRetries: 3, delay: 50 } }, ) dl.on("retry", (...args) => console.debug("RETRY: ", ...args)) dl.on("end", (...args) => console.info("COMPLETED: ", ...args)) dl.on('error', (e) => console.error("ERROR EMITTED: ", e)) try { await dl.start() } catch (e) { console.error(`I GIVE UP\! ${e.message}`) } })() JS ``` Output: ``` RETRY: 1 { maxRetries: 3, delay: 50 } Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } RETRY: 2 { maxRetries: 3, delay: 50 } Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } RETRY: 3 { maxRetries: 3, delay: 50 } Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } ERROR EMITTED: Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } ERROR EMITTED: Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } ERROR EMITTED: Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } ERROR EMITTED: Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } I GIVE UP! getaddrinfo ENOTFOUND nonexist.example.com ```

[Chaphasilor]

Seems to happen because of this change

My guess is the .catch() handler is chained multiple times and after the last retry fails, all (3) catch handlers are invoked, emitting the error event three times...

[hgouveia]

Hello @Envek i just did a new fix, could you try once again?

[Envek]

@hgouveia, now it emits error event only once. Thank you!

Output of the same runnable example on fresh dev branch

``` RETRY: 1 { maxRetries: 3, delay: 50 } Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } RETRY: 2 { maxRetries: 3, delay: 50 } Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } RETRY: 3 { maxRetries: 3, delay: 50 } Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } ERROR EMITTED: Error: getaddrinfo ENOTFOUND nonexist.example.com at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:67:26) { errno: -3008, code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'nonexist.example.com' } I GIVE UP! getaddrinfo ENOTFOUND nonexist.example.com ```

[hgouveia]

@Envek published to npm v1.0.19