Skip to content

fix(auth): recover REST listener and close response on errors#349

Open
edospadoni wants to merge 1 commit into
ns8from
fix-auth-rest-listener-recovery
Open

fix(auth): recover REST listener and close response on errors#349
edospadoni wants to merge 1 commit into
ns8from
fix-auth-rest-listener-recovery

Conversation

@edospadoni
Copy link
Copy Markdown
Member

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 in plugins/util/util.js wrap their body in a try/catch but only log the error from the catch — resp.end() is never called. If resp.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 call resp.end() from their catch block.

Listener not re-bound when connections are hanging

reset() in plugins/com_authentication_rest/server_com_authentication_rest.js called start() only inside the server.close() callback. The Node.js close() 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:

  1. start() is now called immediately after server.close(). The listening socket is released synchronously by close(); only the callback waits for existing connections to drain, so re-binding does not need to wait.
  2. server.closeAllConnections() is invoked when available (Node ≥ 18.2) to force-close lingering keep-alive connections on the old server, so the previous Server instance does not stay around indefinitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant