diff --git a/packages/cacheable-request/src/index.ts b/packages/cacheable-request/src/index.ts index 45e822d3..d5909ae4 100644 --- a/packages/cacheable-request/src/index.ts +++ b/packages/cacheable-request/src/index.ts @@ -315,12 +315,29 @@ class CacheableRequest { ee.emit("error", new CacheError(error)); if (this.cache instanceof Keyv) { const cachek = this.cache; - cachek.once("error", errorHandler); - ee.on("error", () => { - cachek.removeListener("error", errorHandler); - }); - ee.on("response", () => { + // Keyv's event manager wraps once() listeners, so removeListener() + // with the original function would never unregister them. Register + // with on() and remove the same reference on every termination path + // so the listener count stays bounded. + let pendingRequest: any; + const removeErrorHandler = () => { cachek.removeListener("error", errorHandler); + pendingRequest?.removeListener("error", removeErrorHandler); + pendingRequest?.removeListener("abort", removeErrorHandler); + pendingRequest?.removeListener("close", removeErrorHandler); + pendingRequest = undefined; + }; + + cachek.on("error", errorHandler); + ee.on("error", removeErrorHandler); + ee.on("response", removeErrorHandler); + // Aborted or destroyed requests never emit "response", so clean up + // on the underlying request's termination events as well. + ee.on("request", (request_: any) => { + pendingRequest = request_; + request_.once("error", removeErrorHandler); + request_.once("abort", removeErrorHandler); + request_.once("close", removeErrorHandler); }); } diff --git a/packages/cacheable-request/test/cacheable-request-instance.test.ts b/packages/cacheable-request/test/cacheable-request-instance.test.ts index 557a852f..525c6ca8 100644 --- a/packages/cacheable-request/test/cacheable-request-instance.test.ts +++ b/packages/cacheable-request/test/cacheable-request-instance.test.ts @@ -451,3 +451,55 @@ test("cacheableRequest emits CacheError when underlying Keyv emits error event", }); }); }); + +test("cacheableRequest removes Keyv error listener once the request settles", async () => { + const keyv = new Keyv(); + const cacheableRequest = new CacheableRequest(request, keyv).request(); + const baseline = keyv.listeners("error").length; + + const get = async (path: string) => + new Promise((resolve, reject) => { + cacheableRequest(parseWithWhatwg(`${s.url}${path}`), (response: any) => { + response.resume(); + response.once("end", resolve); + }) + .on("error", reject) + .on("request", (request_: any) => request_.end()); + }); + + // Repeated keys are served from cache while fresh keys hit the network; + // neither path may leave its error listener behind (issue #1685) + for (let index = 0; index < 6; index++) { + await get(`/?index=${index % 3}`); + } + + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(keyv.listeners("error").length).toBe(baseline); +}); + +test("cacheableRequest removes Keyv error listener when the request is aborted", async () => { + const keyv = new Keyv(); + const cacheableRequest = new CacheableRequest(request, keyv).request(); + const baseline = keyv.listeners("error").length; + + await new Promise((resolve) => { + cacheableRequest(parseWithWhatwg(s.url)) + .on("error", () => { + /* swallow abort-induced errors */ + }) + .on("request", (request_: any) => { + request_.on("error", () => { + /* swallow abort-induced socket errors */ + }); + request_.once("close", resolve); + request_.abort(); + }); + }); + + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(keyv.listeners("error").length).toBe(baseline); +});