-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
fix(fetch): respect "abort" event on the request signal #394
Changes from 1 commit
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,75 @@ | ||
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.cause.name).toEqual('AbortError') | ||
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.
Show resolved
Hide resolved
|
||
const rejectWhenRequestAborted = new DeferredPromise<string>() | ||
|
||
signal.addEventListener('abort', () => rejectWhenRequestAborted.reject(signal.reason)) | ||
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 think we should implement it the way that user-caused aborts are treated as If I'm reading this right, if the user aborts the request, we don't have to do anything about that except stop the request listener promise. If that's the case, then we should instead have a race between two promises that resolve. But this may be more complicated than that. I'm trying to account for user-caused abort events while working on #398 and I could certainly use your opinion on what's the best way to approach this. 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. We can also use a built-in rejection mechanism Perhaps it'd be more semantically correct to do that and handle the thrown const resolverResult = await until(async () => {
request.signal.throwIfAborted()
// the rest of the request listener promises.
})
if (resolverResult.error) {
if (resolverResult.error instanceof AbortError) {
// We know request listener lookup halted due to
// the operation being aborted.
handleIfNeeded()
return
}
// Handle other exceptions from the request listeners.
} One concern here, as far as I understand, 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. 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. @kettanaito Outside of 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 have updated the PR to use the native error instead of a wrapped error. 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.
We'd have to look how Undici does this. I doubt they implement a Thanks for championing this further! I will take a look once I have a minute. 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. @kettanaito it turns out they do : https://github.com/nodejs/undici/blob/main/test/fetch/abort.js#L78 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 line of code seems to break for me when I'm not setting an AbortController because signal is 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.
When no abort signal is has been provided to the intercepted request, one is injected automatically, so the signal cannot be null. |
||
|
||
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() | ||
|
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.ts
and move it undertest/modules/fetch/compliance
where 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.ts
I 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