-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
test: reproduction of MaxListenersExceededWarning in msw/node
#1910
Conversation
msw/node
The reproduction here showcases 2 separate failure modes:
1st spy call:
2nd spy call:
How does this happen Case 1 - we register 200 handlers, and have a graphql post request to the last one. This results in 201 listeners, not the 200 we expected. This is correctly registering these listeners through the extended Case 2 - we register 200 handlers, and match none of them, but they are a grapqhl request. We copy and parse the request 1x per each hander, then we do it again for the onUnhandedRequest handler for each graphql request, resulting in 303 listeners applied to the cloned request that gets passed to that method, and not the original request |
}), | ||
}).then((response) => response.json()) | ||
|
||
expect(spy).toHaveBeenNthCalledWith(2, 200, expect.any(AbortSignal)) |
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 fail here with just requesting 99 because we're always at a minimum n+1
listeners. 1 per handler + 1 per request.
then we also add one per parse of grapqhl, and we're way over the limit!
Note - the HTTP call above works because order matters. the errors here would flip if we defined the gql handlers first
Math.max(defaultMaxListeners, this.currentHandlers.length), | ||
request.signal, | ||
) | ||
if (typeof setMaxListeners === 'function') { |
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 need this check in jsdom with the mjs file. We don't with cjs, apparently. Not sure why that is offhand
|
||
expect(unhandledResponse.status).toEqual(500) | ||
|
||
expect(requestCloneSpy).toHaveBeenCalledTimes(301) |
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.
These requestClone spies are just to help showcase how many copies we're making, it's a lot!
@mattcosta7, these look very similar to the tests you've added in #1914. Should we close this pull request? |
Yep, this commit was included in that other branch, only left it open to site the failing case |
Released: v2.0.12 🎉This has been released in v2.0.12! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
This test is expected to fail. It shows a scenario where the current logic for
setMaxListeners
fails.relates to #1911
We actually have 2 separate places where this fails currently.
1 - some handlers will call multiple
Request.prototype.clone()
2 - onUnhandledRequest behaviors call another handful of
Request.prototype.clone()
.3 - Some of our clones are of clones, which will append listeners to the intermediate request (it seems)
The caching proposed in #1905 should actually help here as well, since we'll be more resource conscious we won't need to make nearly as many clones and therefore many fewer objects extended