fix(auth): recover REST listener and close response on errors#349
Open
edospadoni wants to merge 1 commit into
Open
fix(auth): recover REST listener and close response on errors#349edospadoni wants to merge 1 commit into
edospadoni wants to merge 1 commit into
Conversation
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
The authentication REST listener can become permanently unresponsive after a single internal reload, requiring a full process restart to recover. This change addresses two independent issues that combine to cause it.
Response not closed on header errors
The
sendHttp*helpers inplugins/util/util.jswrap their body in atry/catchbut only log the error from the catch —resp.end()is never called. Ifresp.writeHead()throws (for example when a value passed to it contains characters not allowed in HTTP header content), the request stays open and the underlying TCP connection is kept alive by the peer.All
sendHttp*helpers now best-effort callresp.end()from their catch block.Listener not re-bound when connections are hanging
reset()inplugins/com_authentication_rest/server_com_authentication_rest.jscalledstart()only inside theserver.close()callback. The Node.jsclose()callback fires only after every existing connection has ended, so any hanging request (such as one left over by the issue above) is enough to prevent it from firing — and therefore prevents the listener from being re-bound to its port indefinitely.Two changes:
start()is now called immediately afterserver.close(). The listening socket is released synchronously byclose(); only the callback waits for existing connections to drain, so re-binding does not need to wait.server.closeAllConnections()is invoked when available (Node ≥ 18.2) to force-close lingering keep-alive connections on the old server, so the previousServerinstance does not stay around indefinitely.