-
-
Notifications
You must be signed in to change notification settings - Fork 168
fix(fetch): respect "abort" event on the request signal #394
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
Changes from 6 commits
52aa901
d48c61a
8e229ca
96e1685
94a2a59
e0ca09d
864a06e
faff151
4753f13
46dd281
e5818a1
f57fcf9
b5e4db9
5470be2
25ad663
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import { DeferredPromise } from '@open-draft/deferred-promise' | ||
| import { HttpServer } from '@open-draft/test-server/http' | ||
| import { afterAll, beforeAll, expect, it } from 'vitest' | ||
| import { FetchInterceptor } from '.' | ||
| import { sleep } from '../../../test/helpers' | ||
|
|
||
| const httpServer = new HttpServer((app) => { | ||
| app.get('/', (_req, res) => { | ||
| res.status(200).send('/') | ||
| }) | ||
| app.get('/get', (_req, res) => { | ||
| res.status(200).send('/get') | ||
| }) | ||
| }) | ||
|
|
||
| const interceptor = new FetchInterceptor() | ||
|
|
||
| beforeAll(async () => { | ||
| interceptor.apply() | ||
| await httpServer.listen() | ||
| }) | ||
|
|
||
| afterAll(async () => { | ||
| interceptor.dispose() | ||
| await httpServer.close() | ||
| }) | ||
|
|
||
|
|
||
| it('abort pending requests when manually aborted', async () => { | ||
| const requestUrl = httpServer.http.url('/') | ||
|
|
||
| interceptor.on('request', async function requestListener() { | ||
| expect.fail('request should never be received') | ||
| }) | ||
|
|
||
| const controller = new AbortController() | ||
| const requestAborted = new DeferredPromise<void>() | ||
|
|
||
| const request = fetch(requestUrl, { signal: controller.signal }) | ||
| request.catch((err) => { | ||
| expect(err.name).toEqual('AbortError') | ||
| expect(err.code).toEqual(20) | ||
| expect(err.message).toEqual('This operation was aborted') | ||
| requestAborted.resolve() | ||
| }) | ||
|
|
||
| controller.abort() | ||
|
|
||
| await requestAborted | ||
| }) | ||
|
|
||
| it('native', async () => { | ||
| interceptor.dispose(); | ||
| const requestUrl = httpServer.http.url('/'); | ||
| const controller = new AbortController(); | ||
| const requestAborted = new DeferredPromise<void>(); | ||
|
|
||
| const request = fetch(requestUrl, { signal: controller.signal }); | ||
| request.catch((err) => { | ||
| expect(err.name).toEqual('AbortError') | ||
| expect(err.code).toEqual(20) | ||
| expect(err.message).toEqual('This operation was aborted') | ||
| requestAborted.resolve() | ||
| }); | ||
|
|
||
|
|
||
| controller.abort(); | ||
| await requestAborted; | ||
| }); | ||
|
|
||
| it('abort ongoing requests when manually aborted', async () => { | ||
| const requestUrl = httpServer.http.url('/') | ||
|
|
||
| const requestEmitted = new DeferredPromise<void>() | ||
| interceptor.on('request', async function requestListener({ request }) { | ||
| requestEmitted.resolve() | ||
| await sleep(10000) | ||
| request.respondWith(new Response()) | ||
| }) | ||
|
|
||
| const controller = new AbortController() | ||
| const request = fetch(requestUrl, { signal: controller.signal }) | ||
|
|
||
| const requestAborted = new DeferredPromise<void>() | ||
|
|
||
| request.catch((err) => { | ||
| expect(err.cause.name).toEqual('AbortError') | ||
| requestAborted.resolve() | ||
| }) | ||
|
|
||
| await requestEmitted | ||
|
|
||
| controller.abort() | ||
|
|
||
| await requestAborted | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { DeferredPromise } from '@open-draft/deferred-promise' | ||
| import { invariant } from 'outvariant' | ||
| import { until } from '@open-draft/until' | ||
| import { HttpRequestEventMap, IS_PATCHED_MODULE } from '../../glossary' | ||
|
|
@@ -46,13 +47,21 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> { | |
|
|
||
| this.logger.info('awaiting for the mocked response...') | ||
|
|
||
| const signal = interactiveRequest.signal | ||
|
kettanaito marked this conversation as resolved.
|
||
| const rejectWhenRequestAborted = new DeferredPromise<string>() | ||
|
|
||
| signal.addEventListener('abort', () => rejectWhenRequestAborted.reject()) | ||
|
|
||
| const resolverResult = await until(async () => { | ||
| await this.emitter.untilIdle( | ||
| const allListenerResolved = this.emitter.untilIdle( | ||
| 'request', | ||
| ({ args: [{ requestId: pendingRequestId }] }) => { | ||
| return pendingRequestId === requestId | ||
| } | ||
| ) | ||
|
|
||
| await Promise.race([rejectWhenRequestAborted, allListenerResolved]) | ||
|
|
||
| this.logger.info('all request listeners have been resolved!') | ||
|
|
||
| const [mockedResponse] = await interactiveRequest.respondWith.invoked() | ||
|
|
@@ -62,10 +71,7 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> { | |
| }) | ||
|
|
||
| if (resolverResult.error) { | ||
| const error = Object.assign(new TypeError('Failed to fetch'), { | ||
| cause: resolverResult.error, | ||
| }) | ||
| return Promise.reject(error) | ||
| return Promise.reject(resolverResult.error) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is incorrect. I copied the previous behavior from Undici and we must comply by it. Note that the FetchInterceptor is primarily meant for Node, and I trust Undici implement the spec rather faithfully.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest we revert this particular change for now because it's not related to the abort controller support. We can discuss it as a separate improvement point, what do you think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked the Undici implementation and there are only two causes of error : abortion and network issue.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, we should do it the same way: handle the two error scenarios separately:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I've done in my latest commit ;)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks so much for addressing it so quickly! Will give it the last round of review and let's get this published. |
||
| } | ||
|
|
||
| const mockedResponse = resolverResult.data | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename this test file to
abort-controller.test.tsand move it undertest/modules/fetch/compliancewhere we store all integration tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same goes for the test added inside
src/interceptors/ClientRequest/index.test.tsI assume ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave that one be, it doesn't concern itself with request handling but focuses on how the
.respondWith()works in the context of the ClientRequest. I think it's fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean I've added a test abort abortion in this file