fix(net): close reactor connections on read error and add a send time… - #5408
Merged
Conversation
…out #5407 HTTPReactorServer left a client abort or a handler failure unhandled in TCPReactorServerConnection::onRead. On a blocking accepted socket a peer reset makes receiveBytes throw; the exception escaped onRead into the reactor and handleClose never ran, so the connection and its fd leaked and the reset fd kept polling readable, hot-spinning the worker reactor until fd exhaustion took the listener down with no log output. Wrap onRead so any exception closes the connection and is logged at error, and collapse the dead negative-count branch into the zero case. Add an opt-in send timeout on TCPServerParams, applied to accepted sockets in the reactor acceptor and defaulting to zero so existing TCPServer users are unchanged. A stalled reader that used to block a reactor thread forever now raises a timeout that closes the connection cleanly. Cover both with tests in the HTTPReactorServer testsuite.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes failure modes in reactor-mode TCP/HTTP servers where exceptions escaping TCPReactorServerConnection::onRead() could prevent cleanup (leaking the connection/fd and hot-spinning the reactor), and adds an opt-in send timeout to bound blocking response writes on reactor threads.
Changes:
- Wrap
TCPReactorServerConnection::onRead()in exception handling that closes the connection on any error and logs at error level; simplify the read-count branching (n <= 0). - Add
TCPServerParams::sendTimeout(default disabled) and apply it to accepted sockets inTCPReactorAcceptor. - Add HTTPReactorServer tests covering client aborts, handler exceptions, and stalled-reader send timeouts.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Net/testsuite/src/HTTPReactorServerTest.h | Declares new regression/feature tests for reactor cleanup and send timeout. |
| Net/testsuite/src/HTTPReactorServerTest.cpp | Implements tests for client abort, handler exceptions, and send-timeout behavior. |
| Net/src/TCPServerParams.cpp | Initializes the new _sendTimeout param to disabled by default. |
| Net/src/TCPReactorServerConnection.cpp | Catches exceptions in onRead(), logs, and closes connections to prevent leaks/spin. |
| Net/src/TCPReactorAcceptor.cpp | Applies the configured send timeout to accepted reactor-mode sockets. |
| Net/include/Poco/Net/TCPServerParams.h | Adds getSendTimeout()/setSendTimeout() API and stores _sendTimeout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…logging #5407 Follow-up to the HTTPReactorServer client-abort fix. A peer reset and a stalled-peer send timeout are routine on a busy server, so onRead now logs those (ConnectionReset, ConnectionAborted, Timeout) at debug and reserves error for genuinely unexpected exceptions, so error stays useful for alerting instead of drowning in benign client churn. Close the connection before logging and wrap the log in a catch-all, so a logging failure can neither skip the cleanup nor escape onRead. Tighten the two liveness tests with short client receive timeouts so a regression fails fast instead of stalling on the default receive timeout, and describe them honestly as crash and hang guards; the send-timeout test remains the behavioral guard.
…RACE #5409 With ENABLE_TRACE every Poco::Exception constructor embeds a stack trace via cpptrace, which symbolizes through libbacktrace's process-global DWARF cache. That cache is not thread-safe, so two exceptions constructed concurrently on different threads race inside it; ThreadSanitizer reports a data race in the libbacktrace symbolizer. ENABLE_TRACE is off by default, so only diagnostic and sanitizer builds are affected. Route the four generate_trace call sites through one helper that holds a single process-global mutex while symbolizing, so concurrent exception construction is serialized. Compiled only when ENABLE_TRACE is set, so default builds are unchanged.
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.
…out #5407
HTTPReactorServer left a client abort or a handler failure unhandled in TCPReactorServerConnection::onRead. On a blocking accepted socket a peer reset makes receiveBytes throw; the exception escaped onRead into the reactor and handleClose never ran, so the connection and its fd leaked and the reset fd kept polling readable, hot-spinning the worker reactor until fd exhaustion took the listener down with no log output.
Wrap onRead so any exception closes the connection and is logged at error, and collapse the dead negative-count branch into the zero case.
Add an opt-in send timeout on TCPServerParams, applied to accepted sockets in the reactor acceptor and defaulting to zero so existing TCPServer users are unchanged. A stalled reader that used to block a reactor thread forever now raises a timeout that closes the connection cleanly.
Cover both with tests in the HTTPReactorServer testsuite.