Skip to content

Commit

Permalink
fix(fromFetch): passing already aborted signal to init aborts fetch
Browse files Browse the repository at this point in the history
* fix(fromFetch): copy init object instead of modifying it

Modifying the object can lead to problems if the init object
is reused in multiple calls to fromFetch.

* fix(fromFetch): cancel fetch if init.signal is already aborted
  • Loading branch information
bgotink authored and benlesh committed Nov 1, 2019
1 parent 52fc393 commit 0e4849a
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 11 deletions.
38 changes: 34 additions & 4 deletions spec/observables/dom/fetch-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@ function mockFetchImpl(input: string | Request, init?: RequestInit): Promise<Res
(mockFetchImpl as MockFetch).calls.push({ input, init });
return new Promise<any>((resolve, reject) => {
if (init.signal) {
if (init.signal.aborted) {
reject(new MockDOMException());
return;
}
init.signal.addEventListener('abort', () => {
reject(new MockDOMException());
});
}
return Promise.resolve(null).then(() => {
Promise.resolve(null).then(() => {
resolve((mockFetchImpl as any).respondWith);
});
});
Expand Down Expand Up @@ -173,13 +177,23 @@ describe('fromFetch', () => {
});

it('should allow passing of init object', done => {
const myInit = {};
const fetch$ = fromFetch('/foo', myInit);
const fetch$ = fromFetch('/foo', {method: 'HEAD'});
fetch$.subscribe({
error: done,
complete: done,
});
expect(mockFetch.calls[0].init.method).to.equal('HEAD');
});

it('should pass in a signal with the init object without mutating the init', done => {
const myInit = {method: 'DELETE'};
const fetch$ = fromFetch('/bar', myInit);
fetch$.subscribe({
error: done,
complete: done,
});
expect(mockFetch.calls[0].init).to.equal(myInit);
expect(mockFetch.calls[0].init.method).to.equal(myInit.method);
expect(mockFetch.calls[0].init).not.to.equal(myInit);
expect(mockFetch.calls[0].init.signal).not.to.be.undefined;
});

Expand All @@ -198,4 +212,20 @@ describe('fromFetch', () => {
// The subscription will not be closed until the error fires when the promise resolves.
expect(subscription.closed).to.be.false;
});

it('should treat passed already aborted signals as a cancellation token which triggers an error', done => {
const controller = new MockAbortController();
controller.abort();
const signal = controller.signal as any;
const fetch$ = fromFetch('/foo', { signal });
const subscription = fetch$.subscribe({
error: err => {
expect(err).to.be.instanceof(MockDOMException);
done();
}
});
expect(mockFetch.calls[0].init.signal.aborted).to.be.true;
// The subscription will not be closed until the error fires when the promise resolves.
expect(subscription.closed).to.be.false;
});
});
18 changes: 11 additions & 7 deletions src/internal/observable/dom/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,18 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab
if (init) {
// If a signal is provided, just have it teardown. It's a cancellation token, basically.
if (init.signal) {
outerSignalHandler = () => {
if (!signal.aborted) {
controller.abort();
}
};
init.signal.addEventListener('abort', outerSignalHandler);
if (init.signal.aborted) {
controller.abort();
} else {
outerSignalHandler = () => {
if (!signal.aborted) {
controller.abort();
}
};
init.signal.addEventListener('abort', outerSignalHandler);
}
}
init.signal = signal;
init = { ...init, signal };
} else {
init = { signal };
}
Expand Down

0 comments on commit 0e4849a

Please sign in to comment.