From 46eaa0ae44b44158c737a26a75dccf7078efa25c Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 9 Feb 2020 16:04:24 +1000 Subject: [PATCH 1/3] test(fetch): add failing test --- spec/observables/dom/fetch-spec.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/observables/dom/fetch-spec.ts b/spec/observables/dom/fetch-spec.ts index 17017c566a..48cb216931 100644 --- a/spec/observables/dom/fetch-spec.ts +++ b/spec/observables/dom/fetch-spec.ts @@ -249,4 +249,19 @@ describe('fromFetch', () => { // The subscription will not be closed until the error fires when the promise resolves. expect(subscription.closed).to.be.false; }); + + it('should not leak listeners added to the passed in signal', done => { + const controller = new MockAbortController(); + const signal = controller.signal as any; + const fetch$ = fromFetch('/foo', { signal }); + const subscription = fetch$.subscribe(); + subscription.add(() => { + try { + expect(signal._listeners).to.be.empty; + done(); + } catch (error) { + done(error); + } + }); + }); }); From d92ca5625cf397f32724f9281eb43a99a56166f1 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 9 Feb 2020 16:17:40 +1000 Subject: [PATCH 2/3] fix(fetch): don't leak signal listeners --- src/internal/observable/dom/fetch.ts | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/internal/observable/dom/fetch.ts b/src/internal/observable/dom/fetch.ts index 50c9df010d..0a7e6ab2b6 100644 --- a/src/internal/observable/dom/fetch.ts +++ b/src/internal/observable/dom/fetch.ts @@ -1,4 +1,5 @@ import { Observable } from '../../Observable'; +import { Subscription } from '../../Subscription'; /** * Uses [the Fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) to @@ -54,10 +55,17 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab return new Observable(subscriber => { const controller = new AbortController(); const signal = controller.signal; - let outerSignalHandler: () => void; let abortable = true; let unsubscribed = false; + const subscription = new Subscription(); + subscription.add(() => { + unsubscribed = true; + if (abortable) { + controller.abort(); + } + }); + let perSubscriberInit: RequestInit; if (init) { // If a signal is provided, just have it teardown. It's a cancellation token, basically. @@ -65,12 +73,14 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab if (init.signal.aborted) { controller.abort(); } else { - outerSignalHandler = () => { + const outerSignal = init.signal; + const outerSignalHandler = () => { if (!signal.aborted) { controller.abort(); } }; - init.signal.addEventListener('abort', outerSignalHandler); + outerSignal.addEventListener('abort', outerSignalHandler); + subscription.add(() => outerSignal.removeEventListener('abort', outerSignalHandler)); } } // init cannot be mutated or reassigned as it's closed over by the @@ -92,11 +102,6 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab } }); - return () => { - unsubscribed = true; - if (abortable) { - controller.abort(); - } - }; + return subscription; }); } From 3aea5eccaa3341ee6a057198fdb52a2d4667b3c1 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 9 Feb 2020 16:23:38 +1000 Subject: [PATCH 3/3] chore: improve an existing test's description --- spec/observables/dom/fetch-spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/observables/dom/fetch-spec.ts b/spec/observables/dom/fetch-spec.ts index 48cb216931..ce914e6942 100644 --- a/spec/observables/dom/fetch-spec.ts +++ b/spec/observables/dom/fetch-spec.ts @@ -206,7 +206,7 @@ describe('fromFetch', () => { expect(mockFetch.calls[0].init!.method).to.equal('HEAD'); }); - it('should pass in a signal with the init object without mutating the init', done => { + it('should add a signal to internal init object without mutating the passed init object', done => { const myInit = {method: 'DELETE'}; const fetch$ = fromFetch('/bar', myInit); fetch$.subscribe({