sindresorhus / got

šŸŒ Human-friendly and powerful HTTP request library for Node.js
MIT License
14.27k stars 935 forks source link

Using CancelableRequest.cancel with cache causes UncaughtException #1925

Closed femshima closed 1 year ago

femshima commented 2 years ago

Describe the bug

import got from "got";

const cache = new Map();
const r = got("https://example.com/", { cache: cache });
r.on('downloadProgress', () => {
    r.cancel();
});

r.catch(e => console.log(e));

In the above code, when I call CancelableRequest.cancel(r.cancel()) with cache in options, ECONNRESET exception is throwed by get-stream package(in addition to CancelError(handled)), which is unhandled even if I put .catch to the returned CancelableRequest.

Error: aborted
    at connResetException (node:internal/errors:691:14)
    at TLSSocket.socketCloseListener (node:_http_client:407:19)
    at TLSSocket.emit (node:events:402:35)
    at TLSSocket.emit (node:domain:475:12)
    at node:net:687:12
    at TCP.done (node:_tls_wrap:580:7)
    at TCP.callbackTrampoline (node:internal/async_hooks:130:17) {
  code: 'ECONNRESET',
  bufferedData: Buffer(0) [Uint8Array] []
}

When I remove cache options, the program only throws CancelError.

According to stacktrace, the exception is throwed in rejectPromise function in get-stream package, called by error callback of pump. https://github.com/sindresorhus/get-stream/blob/main/index.js#L36

const rejectPromise = error => {
    // Don't retrieve an oversized buffer.
    if (error && stream.getBufferedLength() <= BufferConstants.MAX_LENGTH) {
        error.bufferedData = stream.getBufferedValue();
    }
    reject(error);//<-here
};
stream = pump(inputStream, bufferStream(options), error => {
    if (error) {
        rejectPromise(error);
        return;
    }
    resolve();
});

inputStream comes from cacheable-request. https://github.com/lukechilds/cacheable-request/blob/master/src/index.js#L106

Actual behavior

aborted(ECONNRESET) exception is throwed, and it is not handled.

Expected behavior

Cancellation only causes CancelError, not with aborted(ECONNRESET) exception.

Code to reproduce

Runkit: https://runkit.com/femshima/got-cancel-with-cache (For more details, please see https://github.com/femshima/got-cancel-cache)

import got from "got";

const cache = new Map();
const r = got("https://example.com/", { cache: cache });
r.on('downloadProgress', () => {
    r.cancel();
});

r.catch(e => console.log(e));

Checklist

langri-sha commented 2 years ago

I'm encountering the same thing, but I'm not trying to cancel the request, just trying to read the response body.

aubergene commented 2 years ago

I think I have the same issue when the request timeout is triggered

import got from "got"; // version 12.0.3

// 3.5Mb so times out
const url =
  "https://www.interieur.gouv.fr/avotreservice/elections/telechargements/PR2017/resultatsT2/032/002/002com.xml";

got(url, {
  cache: new Map(),
  timeout: {
    request: 500,
  },
})
  .then((res) => {
    if (res.statusCode !== 200) {
      console.error(
        `${res.statusCode} - ${res.statusMessage} - fetching ${url}`
      );
    }

    return res.body.slice(0, 100);
  })
  .catch((err) => {
    console.log("ooooh", err);
  });
node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

Error: aborted
    at connResetException (node:internal/errors:691:14)
    at TLSSocket.socketCloseListener (node:_http_client:407:19)
    at TLSSocket.emit (node:events:402:35)
    at node:net:687:12
    at TCP.done (node:_tls_wrap:580:7) {
  code: 'ECONNRESET',
  bufferedData: Buffer(2424828) [Uint8Array] [
     60,  63, 120, 109, 108,  32, 118, 101, 114, 115, 105, 111,
    110,  61,  34,  49,  46,  48,  34,  32, 101, 110,  99, 111,
    100, 105, 110, 103,  61,  34,  85,  84,  70,  45,  56,  34,
     63,  62,  10,  60,  69, 108, 101,  99, 116, 105, 111, 110,
     62,  10,  32,  32,  60,  83,  99, 114, 117, 116, 105, 110,
     62,  10,  32,  32,  32,  32,  60,  84, 121, 112, 101,  62,
     80, 114, 195, 169, 115, 105, 100, 101, 110, 116, 105, 101,
    108, 108, 101,  60,  47,  84, 121, 112, 101,  62,  10,  32,
     32,  32,  32,  60,
    ... 2424728 more items
  ]
}
jaredwray commented 1 year ago

@aubergene @langri-sha - just FYI that the next version being released in October 22 will have a fix around this. https://github.com/jaredwray/cacheable-request/pull/206

langri-sha commented 1 year ago

@jaredwray thank you šŸ™Œ !!!