Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using CancelableRequest.cancel with cache causes UncaughtException #1925

Closed
2 tasks done
femshima opened this issue Nov 20, 2021 · 4 comments
Closed
2 tasks done

Using CancelableRequest.cancel with cache causes UncaughtException #1925

femshima opened this issue Nov 20, 2021 · 4 comments
Labels
bug Something does not work as it should external The issue related to an external project

Comments

@femshima
Copy link

femshima commented Nov 20, 2021

Describe the bug

  • Got version: 11.8.3
  • Node.js version: 16.13.0
  • OS & version: Windows 10 Home 21H1 19043.1348
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

  • I have read the documentation.

  • I have tried my code with the latest version of Node.js and Got.

@femshima femshima changed the title Using CancelableRequest.cancel with cache causes UnhandledException Using CancelableRequest.cancel with cache causes UncaughtException Nov 21, 2021
@szmarczak szmarczak added bug Something does not work as it should external The issue related to an external project labels Dec 10, 2021
@langri-sha
Copy link

langri-sha commented Feb 25, 2022

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

@aubergene
Copy link

aubergene commented Apr 22, 2022

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
Copy link
Sponsor

@aubergene @langri-sha - just FYI that the next version being released in October 22 will have a fix around this. jaredwray/cacheable#206

@langri-sha
Copy link

@jaredwray thank you 🙌 !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as it should external The issue related to an external project
Projects
None yet
Development

No branches or pull requests

5 participants