MaxListenersExceededWarning in msw/node

[mattcosta7]

Prerequisites

• I confirm my issue is not in the opened issues
• I confirm the Frequently Asked Questions didn't contain the answer to my issue

Environment check

• I'm using the latest msw version
• I'm using Node.js version 14 or higher

Browsers

No response

Reproduction repository

#1910

Reproduction steps

see test setup - #1910

Current behavior

When using some collection of handlers, especially when responding to the last sets of handlers, some warnings may be triggered in node

(node:26291) MaxListenersExceededWarning: Possible EventTarget memory leak detected. abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit

Some notes:

When starting to respond to a request, we set the number of max listeners to the number of handlers currently registered. This assumes each handler makes a single request clone, which is true for http handlers.

GraphQL handlers however, make a request clone per parse because we parse the graphql request body by first cloning it.

When onUnhandledRequest is called we also re-parse and re-clone the request body to check for grapqhl requests again, leading to a minimum of

2x handlers + 2 request clones.

Additionally, some of these parses occur against a clone of a clone, so the abort listeners are defined on both the intiial request and the cloned request leading to potentially an unhandled case of a request clone.

Possible fixes:

1 - Let's make fewer clones, especially of nested requests. This may mean a couple extra clones up front, that get managed from a single location, which at least allows for a less dynamic number of clones to occur (handle this outside of the handlers themselves, and maybe dissallow handlers from 'clone()' calls directly to avoid regression
2 - increase the number of maxListeners allowed. Do we really need to be strict or can we just Infinity the count, and avoid this warning in most cases?

Expected behavior

No warnings occur

[mattcosta7]

Anyone coming across this - the warnings aren't indicative of a real problem, node is a bit protective here for good reason, and we are just not correctly overriding the number of listeners by the actual number of clones for a request.

Everything should work fine, even if you see this warning

[kettanaito]

I suppose this was open after we introduced the setMaxListeners fix in here:

msw/src/node/setupServer.ts

Lines 22 to 25 in ea28bd9

	setMaxListeners(
	Math.max(defaultMaxListeners, this.currentHandlers.length),
	request.signal,
	)

@mattcosta7, does the issue still persist? If so, when does it happen?

1 - Let's make fewer clones, especially of nested requests.

Generally, agree. One has to be careful with cloning and you also voice a solid suggestion to do upfront clones since clones are time-dependent and must be done as early as possible.

2 - increase the number of maxListeners allowed. Do we really need to be strict or can we just Infinity the count, and avoid this warning in most cases?

That's a brute fix. We still want to have Node tell us if there's a potential memory/listener leak. We must set the max listeners count to the request handlers' count, which we are doing right now. I'm thinking if this issue hasn't been solved already.

[mattcosta7]

I suppose this was open after we introduced the setMaxListeners fix in here:

Yes, we're still failing this in many scenarios, and the current tests don't catch the failure because they don't test the last handler

@mattcosta7, does the issue still persist? If so, when does it happen?

Check out - #1910 for the reproduction currently. The test for this checks handlers 42, but not the last handler or the uncaught handler, which are both where the errors are guaranteed to occur.

See - #1914 for potential fix to that, which also removes the need to set listeners.

I think we also would have these messages when using the events api, because more clones might occur in those

1 - Let's make fewer clones, especially of nested requests.

Generally, agree. One has to be careful with cloning and you also voice a solid suggestion to do upfront clones since clones are time-dependent and must be done as early as possible.

This along with caching the clones a bit helps a lot (parseGraphqlRequest makes a clone on every call, for instance)

2 - increase the number of maxListeners allowed. Do we really need to be strict or can we just Infinity the count, and avoid this warning in most cases?

That's a brute fix. We still want to have Node tell us if there's a potential memory/listener leak. We must set the max listeners count to the request handlers' count, which we are doing right now. I'm thinking if this issue hasn't been solved already.

Yep - not suggesting this as the thing we should do, just something we could do

[kettanaito]

Read the pull request. Hyped.

[kettanaito]

Released: v2.0.12 🎉

This has been released in v2.0.12!

• 📄 Release notes
• 📦 npm package

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

---

Predictable release automation by @ossjs/release.