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: abort pending requests when the interceptor is disposed #393

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JesusTheHun
Copy link
Contributor

@JesusTheHun JesusTheHun commented Jul 25, 2023

This PR aims to solve issue mswjs/msw#778.

I took the following approach :

  • Decorate the global AbortController class
  • Hold weak references to all controllers created
  • Once a request is intercepted, hold a strong reference to its controller
  • Once a request ends, free the strong reference
  • On dispose, abort the requests using their controllers

Unhandled requests are not affected.

Since this only affects Node.js, I implemented the logic into FetchInterceptor and ClientRequestInterceptor

@JesusTheHun JesusTheHun changed the title Abort all requests the interceptor is disposed Abort all requests when the interceptor is disposed Jul 25, 2023
@kettanaito kettanaito changed the title Abort all requests when the interceptor is disposed fix: abort pending requests when the interceptor is disposed Sep 2, 2023
@kettanaito
Copy link
Member

Let's have a discussion on this one, I think I know how we can approach it in an elegant manner. Will gather my thoughts and post here once I have a minute.

@JesusTheHun
Copy link
Contributor Author

@kettanaito do you think you can find some time in the coming weeks to handle this PR ? I think I can allocate an afternoon next week if you want to discuss this.

@howagain
Copy link

Wanted to voice my support for this. As being able to test abort controllers is an important part of unit testing. Hoping this PR solves mswjs/msw#1646

@jd-carroll
Copy link

Copying my 2cents here as well...

I think this change really should be in the RequestHandler not the interceptors.

Check out mswjs/msw#778 for a more full discussion.

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented Dec 31, 2023

Wanted to voice my support for this. As being able to test abort controllers is an important part of unit testing. Hoping this PR solves mswjs/msw#1646

@howagain MSW 2.0 includes my first PR that fixes AbortSignal. So if you manually abort the request, you should be able to test that.
If not, this PR won't be enough because what this PR does is to abort all requests during MSW shutdown. It gives you no control while the tests are running.
You may workaround this if you stop and start the server during the test itself, which introduces new issues, if you run yours tests concurrently for example.

This could be a nice improvement to build on top of this PR. Although, given that this PR has not been merged after 5 months, I will not go further before it has been merged.

Maintainers have a limited bandwidth. I understand it can be frustrating but we are working hundreds of hours for the benefits of strangers on the internet. For free.

Be kind ❤️

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

Successfully merging this pull request may close these issues.

4 participants