Open icatalina opened 3 years ago
Hey, @icatalina. Thank you for reporting this.
This appears to be an issue on the library's side, as infinitely pending requests are not terminated when you call server.close()
, while they should be. This shouldn't affect browsers, as the browser destroys the execution context when you reload the page.
We should decide whether this logic belongs to MSW, or to the "interceptors" library, and then design the logic to terminate all pending requests when calling server.close
.
Semantically, interceptors do not "close" anything, they only restore the patched native modules when you call server.close()
:
I think the request termination logic should be on the MSW side, and the interceptors should give the means to terminate a pending request. Alternatively, the interceptors may expose an API like .terminatePendingRequests()
to encapsulate that in a standalone operation.
Diving into the internals a bit more, I can see the termination of the pending requests to be a part of the interceptor.restore()
call and not raise any confusion. The interceptors library should still expose some means to terminate such requests, and it may even call those means internally when calling the .restore()
method.
I've tried to implement a basic request termination for the ClientRequest, but it doesn't work as expected. This likely happens due to the monkey patch that the interceptors library introduces. I've attempted the basic req.destroy()
call that should terminate any ClientRequest in Node.js, but it produces no effect on the monkey patched ClientRequest.
idk if this is off-topic but I also saw this when writing UI tests. I'll add my context below:
I was trying to test the loading state of my UI, I had set the requests to delay('infinite') which worked, but resulted in the same error above Jest did not exit one second after the test has run
.
I realized I don't need to force infinite loading to evaluate the loading UI state, so I ended up doing a fixed 100ms delay and then in the test can evaluate the loading state before waiting for the loading to complete.
@icatalina for example, in your test above you could do something like:
Pseudo code..
beforeEach(() => {
server.use(rest.get('http://localhost/a-request', (_req, res, ctx) => res(ctx.delay(100))));
});
it('shows a loading spinner', async () => {
render(<Component />);
const loadingBar = await findByRole('progressbar'); // maybe don't need to wait?
expect(loadingBar).toBeInTheDocument();
await waitForElementToBeRemoved(() => findByRole('progressbar'));
});
the waitForElementToBeRemoved
came from the "act issue" complaint, which Kent C Dodds talks about the cause and the solution here -> https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning
hi @lpaulger, thanks for the advice. I already did that but used 800ms just to make sure (this is withing the 1s that jest will allow, not ideal, but works). We should still solve the issue with infinite, making the test hang makes it pretty useless.
I wouldn't recommend using waitForElementToBeRemoved
without using some sort of fake timer, it will make your test take 100ms in your case. They can add up pretty quickly and before you know it your test suite takes forever 😅
My bad, I referenced this issue somewhere else 😅
Alternatively, instead of exposing the means to abort a request in Node.js, the library could try terminating the timeout that controls the request execution:
I don't know how that'd treat any pending requests, though. Regardless, there can be a way in the handleRequest
to clear the timeout and forcefully resolve (reject?) the pending request promise in the .resetHandlers
and .close
methods.
Not sure what changed, but 'infinite' now leaves Jest hanging forever... 🤦♂️
Not specially proud of this solution, but for the time being this seems to be working fine for me:
// Delay all unhandled requests forever
const defaultHandler = rest.all('*', (_req, res, ctx) => {
const response = res(ctx.status(501));
return new Promise((resolve) => {
afterEach(() => resolve(response));
});
});
No specially proud of this solution, but for the time being this seems to be working fine for me:
// Delay all unhandled requests forever const defaultHandler = rest.all('*', (_req, res, ctx) => { const response = res(ctx.status(501)); return new Promise((resolve) => { afterEach(() => resolve(response)); }); });
i still have the same problem after this
I think with the new interceptors architecture we should be able to hard-cancel all pending promises on process.on('exit')
and friends. It's a matter of force-resolving/rejecting this Promise queue. At least, this is a good place to start, as all interceptors facilitate AsyncEventEmitter
to await potential listeners.
Contributions are welcome!
@finalight, do you want to try:
rest.all('*', () => new Promise(() => undefined))
I'm not sure if I'm dreaming, but it seems to be working fine 😅
@finalight, do you want to try:
rest.all('*', () => new Promise(() => undefined))
I'm not sure if I'm dreaming, but it seems to be working fine 😅
Doesn't work for me too
Latest iteration, this one seems to be working fine: (don't judge me 🤣)
const emitter = new EventEmitter();
afterEach(() => {
emitter.emit('done');
});
export const handlers = [
rest.all('*', (_req, res, ctx) => {
return new Promise((resolve) => {
listener.once('done', () => {
resolve(res(ctx.status(500)));
});
});
}),
];
That looks interesting, @icatalina, but we should have some internal mechanism for this. The issue with delay('infinite')
is that it creates an infinite setTimeout
, which is what you want (for the response to never arrive) but you want to clean that up.
Effectively, we're introducing a side-effect but we're not cleaning after ourselves. There's no concept of "finished" in the MSW runtime right now because we're not even concerning ourselves with the runtime. The library is request life-cycle-oriented at the moment. A jest test finishing would not affect the request cycle anyway.
This is not an issue for the browser since refreshing the page is enough to garbage-collect all unresolved timeouts. In Node, I'm tempted to do:
process.on('exit', () => resolveSideEffects())
The only problem is that I haven't found a reliable event to react to. "exit" won't be called sometimes. Neither should the process be killed for us to start the cleanup (imagine a --watch
run with Jest). Obviously, we don't want to depend on specific library behaviors like Jest. I'm open to suggestions here.
can't we clean up on server.close()?
@icatalina, interesting. Yeah, I think we could give it a try. How would you track such side-effects like this ctx.delay
timeout? I imagine we need to store them somewhere and go through their cleanup in server.close()
.
I particularly like the Disposable
pattern I've looked up in VS Code. We can adopt it partially:
function createSideEffect() {
// create anything here
// tell how to clean it up
return () => {}
}
So then we could use something like this to push those side-effects into an internal list in the context
and iterate over it in server.close()
, calling the cleanup functions each side-effect returns.
Do you have some ideas around this?
Hey @kettanaito, sorry for the late reply. I'm not sure I follow, I thought the idea was to kill all requests after server.close()
, I'm not sure what kind of ramifications that might have tough 🤔
I think I've run into this same problem using msw with Storybook - I'm trying to set up per-story handlers to display loading and loaded component states similar to this, but once a request has hit a handler that has an infinite delay, subsequent requests to the same url from other stories don't fire at all because the original request is still pending in the browser.
export const LoadedActivity = {
args: { activityId: 1 },
decorators: [
(Story) => {
worker.resetHandlers(
rest.get('/api/v1/activity/:activityId/', (req, res, ctx) =>
res(ctx.json({ id: 1, title: 'My Activity' }))
)
);
return <Story />;
},
],
};
export const LoadingActivity = {
args: { activityId: 1 },
decorators: [
(Story) => {
worker.resetHandlers(
rest.get(
'/api/v1/activity/:activityId/',
(req, res, ctx) => res(ctx.delay('infinite')) // Mock an infinite loading state.
)
);
return <Story />;
},
],
};
So in the example above, once LoadingActivity
has been viewed in the Storybook UI, LoadedActivity
won't render properly because its request to the mocked endpoint never happens - the infinitely delayed one is still Pending so Chrome won't send another.
subsequent requests to the same url from other stories don't fire at all because the original request is still pending in the browser.
I don't think that's how requests in the browser work. Each request is an isolated entry, so dispatching two identical requests will result in two different network entries, regardless of the state of other requests. There's likely a different reason those follow-up requests don't fire, but thanks for mentioning this.
@icatalina, yeah, killing pending requests on server.close()
may be one way to do it. The challenge here is to have an isomorphic way to abort pending requests in Node.js. I think that's natively supported by XMLHttpRequest and ClientRequest, which covers most of the cases in Node.js. It's not supported in fetch
, speaking of the global fetch support, but we may be able to circumvent it by creating an internal AbortController
. My only concern here is how this is supposed to work when the consumer passes a custom AbortController
.
I would love to see someone come up with a prototype for this. This seems to be a feature added to @mswjs/interceptors library. We don't currently keep any track of what requests are performed but it looks like we need to do that now in order to do requests.map(r => r.abort())
on the global request abort thing. Keeping track of requests on MSW's side is also possible but we still need some means from the interceptors library to perform the request cancellation. Because of that, it makes sense to move the whole logic to the lowest common ground, being the interceptors library.
Here's an approximate way we can approach this.
We have a Interceptors.prototype.dispose
method that's called when disposing of each interceptors (read "server.close()")"
We can extend this method in each particular interceptor and implement request cancellation.
// src/interceptors/ClientRequest/index.ts
class ClientRequestInterceptor extends Interceptor {
private pendingRequests: Map<string, Request>
protected setup() {
// ...the existing setup here
// Keep all dispatched requests internally.
this.emitter.on('request', (request, requestId) => {
this.pendingRequests.set(requestId, request)
})
// Potentially remove requests from the pending requests map.
// The problem here is that "response" is not the only way
// a request may settle. It may also error. Maybe hooking into
// the "response" event is unnecessary, and we should instead
// check the request state in "this.abortPendingRequest" before
// aborting the request.
this.emitter.on('response', (response, request, requestId) => {
this.pendingRequests.delete(requestId)
})
}
public dispose() {
super.dispose()
this.abortPendingRequests()
}
private abortPendingRequests() {
// Go through every request that's still pending
// and abort it.
for (const [requestId, request] of this.pendingRequests) {
request.abort()
}
}
}
Moving the request cancellation logic to each individual interceptor makes sense as the base Interceptor
class is rather generic and can implement any interceptors (e.g. WebSocket events interception that has no "abort" by definition).
hi @kettanaito I ran into this at the work the other day and would love to take this on if no one else is already working on it. I'm pretty new to the msw libraries but was doing some digging yesterday based on the above discussion and wanted to follow-up with some thoughts.
the problem I saw with using the request
/response
hooks is that we get the InteractiveRequest
which I'm not sure how to abort, since what we actually need is the physical ClientRequest
and/or XMLHttpRequest
to call .destroy()
and .abort()
on respectively as far as I can tell. (let me know if I am missing something here though since like I said this is a bit new to me 😊)
assuming the above is correct, was wondering what you thought about emitting a new request-internal
event that gives us the actual "internal" request instances we need for this with their associated ids to consume or providing an internal setter to register new internal requests with the interceptor. Alternatively, I was messing around with overriding the signal property in NodeClientRequest
like so
// src/interceptors/ClientRequest/NodeClientRequest.ts
...
constructor(
[url, requestOptions, callback]: NormalizedClientRequestArgs,
options: NodeClientOptions // extended to take abort controller from interceptor
) {
const requestOptionsWithSignalOverride = {
...requestOptions,
signal: options.requestController.signal,
}
// if user tries to abort using passed signal setup listener to call signal override to abort
function onAbort() {
options.requestController.abort()
requestOptions.signal?.removeEventListener('abort', onAbort)
}
requestOptions.signal?.addEventListener('abort', onAbort)
super(requestOptionsWithSignalOverride, callback)
...
that would then allow the interceptor to call requestController.abort()
when dispose
is called while preserving any user passed signals but I wasn't sure if that would work as nicely for XMLHttpRequest
(although I did see some examples about how we might make that work). The abort controller approach is also not as extenisble though if we want to do anything else with the pending requests being tracked with the first approach in the future I suppose.
I just ran into this issue as well. It'd be great to have support for this as currently quite a lot of my tests have to use an arbitrary delay which makes them flaky.
Hey, @russomp. Thanks for showing interest in working on this! Let's go through the points you've mentioned.
the problem I saw with using the request/response hooks is that we get the InteractiveRequest which I'm not sure how to abort, since what we actually need is the physical ClientRequest and/or XMLHttpRequest to call .destroy() and .abort() on respectively as far as I can tell.
That's correct, and the Interceptors used to expose a reference to the request instance but they don't do that anymore. Anyhow, MSW shouldn't handle this directly, but instead, the Interceptors must expose an API to do that implicitly do that for us, as the references on that level are still available and it's the Interceptors that should have an API to abort any pending requests.
assuming the above is correct, was wondering what you thought about emitting a new request-internal event that gives us the actual "internal" request instances
I think this inverses the control if I understand your suggestion correctly. If you mean this:
consumer -> emit "request-internal" -> get request refs -> call ref.abort()
Then the control flow here is incorrect. We already have consumer (MSW) / library (Interceptors) communication in a form of interceptors
API. So, I think no changes are needed on MSW's side to try to solve this. The Interceptors library needs to implicitly kill any pending requests of the interceptor's kind upon interceptor.dispose()
of the respective interceptor.
Your signal based-take is interesting but I'm not sure to what extent we can reuse that across different interceptors (XHR/fetch). The way I would recommend approaching this implementation is to stay request module-agnostic, so what we come up with works for any interceptor the very same way. That's why using the Interceptor
class as the common ground for this sounds like a good initial idea:
emitter
that manages request/response events.dispose()
, the interceptor goes through each stored request and performs request.abort()
(or other, based on the intercepted request type). What remains as a question is whether this behavior can be safely applied to all interceptors. Whether you'd always want to abort pending requests when disposing of an interceptor. In other words, to think about the cases when you wouldn't want that to happen. I can't think of any at the top of my head right now.
This relation would work because the following call stack occurs at the end of a test run:
-> test runner: afterAll()
-> MSW: server.close()
-> Interceptor(s): [i].dispose()
@finalight, do you want to try:
rest.all('*', () => new Promise(() => undefined))
I'm not sure if I'm dreaming, but it seems to be working fine 😅
This actually worked for me 🙈
@kettanaito I've submitted a PR into mswjs/interceptors
to fix this issue. Would you mind taking a look ?
My 2cents here...
Instead of putting this on the interceptors, I think it should be placed in the RequestHandler
. I have a working solution locally that does two things:
1) Calling .close()
, .dispose()
, .restoreHandlers()
, or .resetHandlers()
on the server should abort any active request
2) Provides a mechanism for the server to have a new api of server.allActive()
where you could do something like await server.allActive()
-> (which can be super useful if you're testing with jest)
I'm happy to pull together a PR for these changes, but it touches all of the core API's...
So if this doesn't sound of interest or there's a low probability of it getting merged I'd love to focus on the things I should be. :stuck_out_tongue:
Again, happy to throw together PR quick, but it is not fully implemented / tested nor would I have the bandwidth to get it across the finish line by myself.
@jd-carroll can you articulate why it would be better on RequestHandler
?
My primary use case was await
'ing all active requests being handled by MSW, not necessarily being able to cancel any active request. And RequestHandler
seemed like a good, centralized, location to implement a hook to solve that.
I've copied my implementations of RequestHandler
and an updated SetupServerApi
(aka. AwaitableServerApi
) below.
The core concept is creating a "registration" for each request and then sharing that with the server. The registration looks like:
const registration: HandledRequestRegistration = {
handler: this,
status: 'pending',
abort: () => ac.abort(),
promise
};
Maybe it would be better to move this into SetupApi
, but I am not familiar enough with the browser service worker implementation to know whether or not that'd even be possible. (And if its in SetupApi, I could also see an argument for moving to the interceptors.)
The implementations are not perfect, but they met my very basic needs.
Lastly, I added some abort controller logic that may be very wrong. I have not tested it fully, nor tested whether or not it is necessary (it may not be).
@jd-carroll can you share an example of usage within a test ?
@JesusTheHun Not sure that sharing the test case would provide much insight. But happy to elaborate on any aspect of the implementation / thought process if that would help.
@jd-carroll I want to see the API / DX
:eyes:
Describe the bug
⚠️ I'm not sure if this is the correct repo to report this.
When trying to use
delay('infinite')
to mock loading states, jest throws a warning and hangs forever.This happens because the promises hang forever, but I can't find a way to cancel/reject/terminate them. I'm not really sure what's the correct way of handling this.
Environment
msw: 0.29.0
nodejs: 14.17.0
npm: 7.15.1
To Reproduce
Just to give some extra context, the real code is more like:
Expected behavior
Requests are aborted(?), the tests conclude and jest doesn't warn about opened handlers.
Screenshots