Repository navigation
fix(dev-proxy): stop leaking an error listener and a pending promise per request - #486
Open
Black-Break wants to merge 1 commit into
Open
Black-Break wants to merge 1 commit into
Black-Break wants to merge 1 commit into
Conversation
…per request
The proxy route registered `proxy.once('error', reject)` inside the handler
and returned a `new Promise` that was never settled.
Neither could ever happen. `proxy.web()` only routes upstream errors to its
callback when one is supplied -- see the `if (clb) { clb(err, ...) } else {
server.emit('error', ...) }` branch in
lib/http-proxy/passes/web-incoming.js -- so the `'error'` event is never
emitted and the `once` listener is never removed. The shared module-level
proxy server therefore gained one listener per request until Node's default
maxListeners of 10 tripped MaxListenersExceededWarning.
On the success path the callback is never invoked at all (the response is
piped directly), so `resolve` was never called and every request left a
pending promise and its closure behind.
Register a single module-level `'error'` listener instead: it keeps the
listener count flat and still covers the one path that does emit -- `web()`
with a missing or invalid target (lib/http-proxy/index.js) -- where an
unhandled `'error'` event would otherwise crash the process. The handler now
returns void, which is what `externalResolver: true` expects anyway.
Listener count over 20 requests: 1 -> 20 before, 1 -> 1 after.
Adds three tests covering the shared server, the flat listener count, and the
handler settling its return value.
Signed-off-by: Black-Break <134817940+Black-Break@users.noreply.github.com>
|
@Black-Break is attempting to deploy a commit to the codersett's projects Team on Vercel. A member of the Team first needs to authorize it. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/pages/api/development/proxy/index.tsregisteredproxy.once('error', reject)inside the request handler and returned anew Promisethat was never settled. Neither of those could ever happen, so the route leaked one event listener and one pending promise on every single request.Fixes #.
The bug
http-proxy@1.18.1routes upstream errors to the callback you pass toproxy.web(), and only emits'error'when you don't pass one —lib/http-proxy/passes/web-incoming.js:The route passes a callback, so
'error'is never emitted. That makes both halves of the original code dead:proxy.once('error', reject)never fires, so it is never removed. The proxy server is a module-level singleton, so the listener count grows by one per request and trips Node's defaultmaxListenersof 10.The returned promise never settles. The callback is invoked only on failure (on success the response is piped directly at
web-incoming.js:187), and even on failure it only didconsole.log— it never calledresolveorreject. Every request therefore left a pending promise and its closure behind.Measured with the added test, over 20 requests: listener count went
1 → 20. Node warns at 11.The fix
proxy.on('error', ...)instead of a per-requestonce. That keeps the count flat, and it still covers the one path that does emit —web()with a missing or invalid target (lib/http-proxy/index.js:69), where an unhandled'error'event would otherwise crash the process.voidfrom the handler.externalResolver: truemeans Next.js does not wait on the return value, andhttp-proxywrites the response itself, so there is nothing to await.Listener count over 20 requests:
1 → 1.Verification
Before the fix, both behavioural tests fail on their assertions:
Full suite and the other CI gates:
The single failing test is pre-existing and unrelated:
src/modules/intelligent-analysis/server/pathSafety.test.tsasserts against the POSIX path/srv/compass/public/test/..., which cannot resolve on Windows. It fails identically on an unmodified checkout.next buildcannot complete on this Windows machine — it fails withEMFILE: too many open filesduringCollecting build traces. I confirmed this reproduces on a cleanmainwith zero modifications, so it is environmental and not caused by this change.