Skip to content
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

Merged
merged 15 commits into from
Sep 2, 2023

Conversation

JesusTheHun
Copy link
Contributor

This discussion pointed out that fetch request are not aborted if the user abort them using an AbortController.
This PR fixes it.

const signal = interactiveRequest.signal
const rejectWhenRequestAborted = new DeferredPromise<string>()

signal.addEventListener('abort', () => rejectWhenRequestAborted.reject(signal.reason))
Copy link
Member

Choose a reason for hiding this comment

The 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 AbortError, as per spec.

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.

Copy link
Member

@kettanaito kettanaito Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also use a built-in rejection mechanism .throwIfAborted on the signal (spec).

Perhaps it'd be more semantically correct to do that and handle the thrown AbortError as a special case below.

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, .throwIfAborted() is a one-time action. If the request wasn't aborted by the time this method is called, it will never throw even if the request will be aborted in the future.

Copy link
Contributor Author

@JesusTheHun JesusTheHun Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address your points in the near future but I wanted to let you know that I already have submitted a PR for #398

Edit : see PR #393

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kettanaito Outside of msw realm, when a request is aborted, the request/promise is rejected with a DOMException that have reason set to AbortError. Is there any reason to change this behavior ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of msw realm, when a request is aborted, the request/promise is rejected with a DOMException that have reason set to AbortError. Is there any reason to change this behavior ?

We'd have to look how Undici does this. I doubt they implement a DOMException just for this sake. I'm tempted to say they rely on a regular error with the cause set. I'm not sure where I'm proposing something related to that, please point me to that.

Thanks for championing this further! I will take a look once I have a minute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The 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 null in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 null in that case.

When no abort signal is has been provided to the intercepted request, one is injected automatically, so the signal cannot be null.
If you are facing any issue, please open one.

@@ -0,0 +1,96 @@
import { DeferredPromise } from '@open-draft/deferred-promise'
Copy link
Member

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 under test/modules/fetch/compliance where we store all integration tests.

Copy link
Contributor Author

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 ?

Copy link
Member

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.

Copy link
Contributor Author

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

cause: resolverResult.error,
})
return Promise.reject(error)
return Promise.reject(resolverResult.error)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Since the abortion error is well defined (specific class and specific error code), we can check the error type and reject accordingly.
If we don't do that, we deviate from production behavior.

Copy link
Member

Choose a reason for hiding this comment

The 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:

  • Keep what you've introduced for abort errors.
  • Revert what was there previously (the TypeError) to handle all the other errors (effectively, network errors).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I've done in my latest commit ;)

Copy link
Member

Choose a reason for hiding this comment

The 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.

@kettanaito
Copy link
Member

I've updated the tests a little, but what you wrote was really good. The abort controller tests now pass but I think we need to take a look at the test/modules/fetch/fetch-exception.test.ts to see if I got the original intention there wrong.

During my tests with Undici, network errors would result in a TypeError: Failed to fetch where the original thrown exception is set in the cause by Undici (I don't believe that happens in the browser). In other aspects, a network error also results in TypeError in the browser.

Here's the reference: https://github.com/nodejs/undici/blob/111fd2388094270259814ca7ea5da7d07b2126f6/lib/fetch/index.js#L227

Note that Unidici treats response (?) abort and network errors differently:

https://github.com/nodejs/undici/blob/111fd2388094270259814ca7ea5da7d07b2126f6/lib/fetch/index.js#L211-L221

Maybe we should do the same in the resolverResult.error handling.

const signal = interactiveRequest.signal
const requestAbortRejection = new DeferredPromise<string>()

signal.addEventListener('abort', () => requestAbortRejection.reject())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we forward the abort information exposed to this listener, like abort reason and such? It does work without it so I assume the abort controller already exposes that information in the abort error, is that correct?

As in, why this is not needed?

signal.addEventListener('abort', () => {
  requestAbortRejection.reject(signal.reason)
})

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out it does not work.
The reason your test passes is because you abort the request before it starts. If you try the same thing with a pending request, abortError will be undefined.

If the request is aborted, the condition line 73 is not fulfilled, the flow will therefore continue and execute the native request, which will throw because it has been aborted. This is why it looks like it works.

Please also note that the condition line 73 is incorrect : if null is given as a reason, the condition will fails and the flow will continue. While this do not lead to a runtime issue as explained above, I changed the condition to also check if the abortion promise was rejected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand how it affects the promise flow, us using the abort reason as the rejection value and that it may produce false negatives, but my question was about whether we have to respect that reason in any way? As in, forward it directly. We can always reject with an object, or a different data structure to prevent false positives.

reject with the abortion reason or reject with a TypeError
@@ -70,10 +70,17 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> {
return mockedResponse
})

if (resolverResult.error || requestAbortRejection.state === 'rejected') {
if (requestAbortRejection.state === 'rejected') {
Copy link
Member

@kettanaito kettanaito Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should conceptually treat this promise as resolved instead?

If we imagine promise as an intention, then the intention here is to know when the request has been aborted by another actor. In that light, an aborted request would mean a resolved requestAborted promise.

It would make this check read much nicer as well. What are your thoughts on this?

I'm also a bit hesitant to base this condition on the request abort promise itself. I'd rather we used that promise in the Promise.race() but handled the resolverResult.error differently. This implies that requestAbortRejection can still throw but it does so with a custom error/object that is later checked on the resolverResult.error:

signal.addEventListener('abort', () => {
  requestAbortRejection.reject({ type: 'aborted', reason: signal.reason })
})

if (resolverResult.error) {
  if (resolverResult.error.type === 'aborted') {
    return Promise.reject(resolverResult.error.reason)
  }

  return /* create the TypeError, set "cause" */
}

We can also use a custom AbortError class to prevent any false positives here since it's possible for the user to abort a request with a compatible object as the reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is true that the promise encapsulate the expectation of an aborted request.
Resolving it would mean that the expectation has been reached, which is logically correct.
However, in our case, we use a promise as a poor man's event : the promise rejection carries no expectation and therefore no logical meaning. We just want to be notified when the abortion happens.
From there, this promise is a mere async tool.
Resolving it, then check the race winner, then throw a custom object, check the resolverResult resolution value, all of that to ultimately throw the error we had in the first place, seems unnecessary to me.
On a side note, in the end, I'm not the maintainer of the project, if you feel more comfortable with this extra layer, do it, this is your call :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input. I agree that it's mainly a semantic difference. We can leave it rejecting as-is. On the second half, I will lean toward making the resolver handling more streamlined, handling it by what the until returns instead of relying on external factors like the rejection promise. I will push the change shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my point is that this is not a semantic difference but a technical one. We expect something to happen, using resolve or reject make no difference. If our expectation could fail then it would make a semantic difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up leaving the explicit rejection promise check but instead of coupling the request rejection promise and the resolver error, I reject the request promise with the abort rejection reason. Also, that is hell of a sentence to write.

@kettanaito kettanaito changed the title fix: abort request if the user aborts it fix(fetch): abort request if the user aborts it Sep 1, 2023
@kettanaito kettanaito changed the title fix(fetch): abort request if the user aborts it fix(fetch): respect "abort" event on the request signal Sep 1, 2023
@kettanaito
Copy link
Member

@JesusTheHun, hi! The changes look great from my side, is there anything else we should consider before publishing this?

() => {
requestAborted.reject(signal.reason)
},
{ once: true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made sure we only react to the abort event once to make sure there's no event emitter memory leak when cloning requests (each request clone will also preserve previously attached listeners, I believe; I recall an issue about this in the past).

Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. Thank you for your contribution, @JesusTheHun! 👏

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented Sep 1, 2023

LGTM 👌 Now #393 😁💪

@kettanaito
Copy link
Member

There's an issue with the automated releases in this repo, I will look into it and see if I can figure it out before releasing this. If I notice it taking much time, I will release manually but I'd pretty much love to solve the automation issue.

@kettanaito kettanaito merged commit 33dcbe0 into mswjs:main Sep 2, 2023
1 check passed
@kettanaito
Copy link
Member

Released: v0.24.1 🎉

This has been released in v0.24.1!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants