dl.pause() and dl.stop() not working properly on Node 16

Chaphasilor commented 3 years ago

When using the package with Node 16, calling dl.pause() or dl.stop() will result in the http response emitting an error event, which in turn causes dl to emit an error event as well. With dl.stop() this isn't a big deal (although still unexpected), but with dl.pause() this results in a retry, which means that after waiting for retry.delay, the download will be resumed instead of staying paused.

After "some" investigating, I've found out the following:

the original error is emitted by the http response
the reason is emitted seems to be that the http request is being canceled, which leads to the response socket to be destroyed
the error can be avoided by canceling the response before canceling the request, e.g. by calling response.destroy()
request.abort(), which is currently being used is deprecated and should be replaced with request.destroy() (which does the same)

Here's some code for recreating the issue:

Error when calling `dl.pause()` or `dl.stop()`

```js const { DownloaderHelper } = require('./dist') const dl = new DownloaderHelper('http://samples.mplayerhq.hu/4khdr/LaLaLand_cafe_4K.mkv', __dirname, { fileName: 'test', }); dl.start().catch(err => { console.error(`Download failed:`, err) }) dl.on(`error`, error => { console.warn(`error:`, error) }) dl.on(`download`, () => { setTimeout(async () => { let success = await dl.pause() // let success = await dl.stop() // uncomment me if (success) { console.log(`PAUSED`) } else { console.log(`FAILED`) } }, 2500) }) dl.on(`retry`, (attempt, retryOpts) => { console.log(`RETRYING`) console.log(`attempt:`, attempt) console.log(`retryOpts:`, retryOpts) }) dl.on(`progress.throttled`, progress => { console.log(`progress:`, progress) }) process.stdin.on(`data`, data => { console.log(`data:`, data) }) ```

Error (and fix) for the `http` module

```js const http = require(`http`); const fs = require(`fs`); const url = "http://samples.mplayerhq.hu/4khdr/LaLaLand_cafe_4K.mkv"; let urlObj = new URL(url) const requestOptions = { protocol: urlObj.protocol, host: urlObj.hostname, port: urlObj.port, path: urlObj.pathname, method: `GET`, }; let response const request = http.request(requestOptions, res => { response = res; console.log(`response received`) console.log(`statusCode: ${res.statusCode}`) if (res.statusCode == 200) { res.on(`data`, () => process.stdout.write(`.`)) res.on(`error`, err => console.error(`RESPONSE ERROR:`, err)) const fileStream = fs.createWriteStream(`test.file`, {}); res.pipe(fileStream) } else { console.error(`Bad response`); } }); request.on('error', error => { console.error(`request error:`, error) }) request.end() setTimeout(() => { // cancel response *before* canceling request // response.destroy(); //!!! uncomment to fix error request.destroy() // or `request.abort()` }, 2500) ```

It should be a fairly easy fix, but I'd prefer not to contribute it myself because I'm not super-familiar with how the events and request/reponse streams should be handled.
If you really don't have the time to fix it yourself, it would be very appreciated if you could give me a rough outline of how and where I should change things :)

hgouveia commented 3 years ago

@Chaphasilor thanks for you very deep inside of this!, do not worry i will take care because sadly this cant just be replaced, since this needs to be compatible with older node version as well to not break compatibility for the apps already dependent

Chaphasilor commented 3 years ago

Just a heads-up that request.abort() has been deprecated since Node 14 and will likely be completely removed in Node 17 or 18...

Releasing a new major version on npm also wouldn't break any old projects using the library, and you could discontinue support for Node <14 at some point ([maybe when Node 12 reaches end-of-life, which is on 2022-04-30)(https://nodejs.org/en/about/releases/)).

Anyway, replacing request.abort() with request.destroy() isn't really needed to fix this issue, the only thing needed (as far as I can tell) is calling response.destroy() before request.abort().
There also is no response.abort(), only response.destroy(), so there shouldn't be any compatibility issues :)

hgouveia commented 3 years ago

hello @Chaphasilor i just did a fix not related, but it seems also fixed this, could you check? i've checked pause/resume with node 16 and didnt emitted the error, i will do the fixes you suggested anyways, but i am curious

Chaphasilor commented 3 years ago

chaphasilor@Chaphasilor-Spectre:/mnt/d/Code/misc/node-downloader-helper$ node tmp
progress: {
  total: 446164495,
  name: 'test (1) (1)',
  downloaded: 1258281,
  progress: 0.2820217686752506,
  speed: 1258281
}
progress: {
  total: 446164495,
  name: 'test (1) (1)',
  downloaded: 2558601,
  progress: 0.5734658469405998,
  speed: 1300320
}
error: Error: aborted
    at connResetException (node:internal/errors:691:14)
    at Socket.socketCloseListener (node:_http_client:407:19)
    at Socket.emit (node:events:406:35)
    at TCP.<anonymous> (node:net:672:12) {
  code: 'ECONNRESET'
}
PAUSED
Download failed: Error: aborted
    at connResetException (node:internal/errors:691:14)
    at Socket.socketCloseListener (node:_http_client:407:19)
    at Socket.emit (node:events:406:35)
    at TCP.<anonymous> (node:net:672:12) {
  code: 'ECONNRESET'
}
^C
chaphasilor@Chaphasilor-Spectre:/mnt/d/Code/misc/node-downloader-helper$ node -v
v16.7.0

I pulled your latest commits (especially 9d1bd9ceb9a259e1a798f4b2c7fc9d2f1e147ec6) from the dev branch, but I'm still getting the errors and the download still fails after consuming all the retries. Not happening with Node 14.

hgouveia commented 3 years ago

@Chaphasilor cool thanks, it seems it behaves differently in linux, i will check again on monday

hgouveia commented 3 years ago

@Chaphasilor i did the changes you suggested , hopefully will work now, could you try? thanks

Chaphasilor commented 3 years ago

@hgouveia that seems to have done the trick, awesome! :D

please let me know once it hits npm :)

Luury commented 3 years ago

Update npm please, I had the same issue :/

Chaphasilor commented 3 years ago

@hgouveia Any ETA on the next release? would be really great if this would finally hit npm :)

hgouveia commented 3 years ago

hello @Chaphasilor , sorry the delay i will try to publishing today or tomorrow

hgouveia commented 3 years ago

@Chaphasilor published to npm v1.0.19

aleksey-hoffman commented 3 years ago

@hgouveia thank you for updating the module, I'm considering switching to it in my file manager app.

However, I found that pause / stop / resume is not working on Node 15 (It's working fine in Node 14 and 16):

Code

const { DownloaderHelper } = require('node-downloader-helper')
const fileUrl = 'https://ftp.nluug.nl/pub/graphics/blender/release/Blender2.93/blender-2.93.4-windows-x64.msi'
const dl = new DownloaderHelper(fileUrl, 'E:\\test')

dl.on('end', () => console.log('Download Completed'))
dl.on('progress', (stats) => console.log(stats))
dl.start()

setTimeout(() => {
  dl.pause()
  setTimeout(() => {
    dl.resume()
  }, 1000)
}, 1000)

Error

node:internal/errors:642
  const ex = new Error(msg);
             ^

Error: aborted
    at connResetException (node:internal/errors:642:14)
    at TLSSocket.socketCloseListener (node:_http_client:424:27)
    at TLSSocket.emit (node:events:381:22)
    at node:net:666:12
    at TCP.done (node:_tls_wrap:577:7)
Emitted 'error' event on b instance at:
    at E:\test-projects\filedownloader\node_modules\node-downloader-helper\dist\index.js:1:10136 {
  code: 'ECONNRESET'
}

hgouveia commented 3 years ago

@hgouveia thank you for updating the module, I'm considering switching to it in my file manager app.

However, I found that pause / stop / resume is not working on Node 15 (It's working fine in Node 14 and 16):

Code

const { DownloaderHelper } = require('node-downloader-helper')
const fileUrl = 'https://ftp.nluug.nl/pub/graphics/blender/release/Blender2.93/blender-2.93.4-windows-x64.msi'
const dl = new DownloaderHelper(fileUrl, 'E:\\test')

dl.on('end', () => console.log('Download Completed'))
dl.on('progress', (stats) => console.log(stats))
dl.start()

setTimeout(() => {
  dl.pause()
  setTimeout(() => {
    dl.resume()
  }, 1000)
}, 1000)

Error

node:internal/errors:642
  const ex = new Error(msg);
             ^

Error: aborted
    at connResetException (node:internal/errors:642:14)
    at TLSSocket.socketCloseListener (node:_http_client:424:27)
    at TLSSocket.emit (node:events:381:22)
    at node:net:666:12
    at TCP.done (node:_tls_wrap:577:7)
Emitted 'error' event on b instance at:
    at E:\test-projects\filedownloader\node_modules\node-downloader-helper\dist\index.js:1:10136 {
  code: 'ECONNRESET'
}

Hello @aleksey-hoffman , thanks for the report i will check this out as soon I can

hgouveia commented 2 years ago

@aleksey-hoffman i was checking this out with node 15 i didnt found any solution yet, is this still happening to you?

aleksey-hoffman commented 2 years ago

@hgouveia not working on 15.14.0. Using Node 16.x seems like a good solution. It doesn't look like many people are having issue with this, so don't worry about it.

hgouveia commented 2 years ago

@aleksey-hoffman cool thank! i will close it for now, if this happens in future version of node, please open it again i will try to find out the issue!

hgouveia / node-downloader-helper