fix(net): preserve exception type in HTTPReactorServer::onError #5407 - #5410
Merged
Conversation
onError takes a const Poco::Exception& and rethrew it with throw ex, which copy-constructs a base Poco::Exception and slices the dynamic type away. Any downstream classification by type then fails silently: TCPReactorServerConnection::onRead dynamic_casts for ConnectionReset, ConnectionAborted and Timeout so routine client disconnects log at debug and only genuine faults log at error. A sliced exception matches none of them and falls through to the error branch, so ordinary client churn is reported as a fault. Use ex.rethrow(), which is virtual and implemented by POCO_DECLARE_EXCEPTION as throw *this, preserving the derived type. Also remove the unused HTTPReactorServer::_threadPool member. It was never referenced, and the ThreadPool constructor eagerly starts minCapacity threads, so every server instance spawned two threads that never received work. Its presence also implied a dispatch pool that does not exist: request handlers run inline on the reactor worker thread. Note this changes the class layout, so dependents linking PocoNet need a rebuild. Fix the _wokerReactors misspelling in TCPReactorAcceptor. Adds testOnErrorPreservesExceptionType, which asserts a Net exception, a Foundation exception and a plain Poco::Exception all survive onError with their type intact. Verified to fail with the sliced form and pass with rethrow.
…5407 testOnErrorPreservesExceptionType compared an exception message to a string literal. Under ENABLE_TRACE the Poco::Exception constructor appends a captured stack trace to _msg (Foundation/src/Exception.cpp), so message() reads plain\n<trace> there and the equality fails. Every CI job builds with ENABLE_TRACE=ON, so all 12 failing checks were this single assertion - including windows-2025-msvc-cmake, which runs no sanitizer at all. No ASan, UBSan or TSan report appears in any of the logs. Assert the prefix instead. That still proves the payload survives rethrow(), and holds both in a default build and in a trace build. Verified by configuring a second tree with ENABLE_TRACE=ON, reproducing the exact failure and line, then confirming both configurations pass.
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
1 participant
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.
onError takes a const Poco::Exception& and rethrew it with throw ex, which copy-constructs a base Poco::Exception and slices the dynamic type away. Any downstream classification by type then fails silently: TCPReactorServerConnection::onRead dynamic_casts for ConnectionReset, ConnectionAborted and Timeout so routine client disconnects log at debug and only genuine faults log at error. A sliced exception matches none of them and falls through to the error branch, so ordinary client churn is reported as a fault.
Use ex.rethrow(), which is virtual and implemented by POCO_DECLARE_EXCEPTION as throw *this, preserving the derived type.
Also remove the unused HTTPReactorServer::_threadPool member. It was never referenced, and the ThreadPool constructor eagerly starts minCapacity threads, so every server instance spawned two threads that never received work. Its presence also implied a dispatch pool that does not exist: request handlers run inline on the reactor worker thread. Note this changes the class layout, so dependents linking PocoNet need a rebuild.
Fix the _wokerReactors misspelling in TCPReactorAcceptor.
Adds testOnErrorPreservesExceptionType, which asserts a Net exception, a Foundation exception and a plain Poco::Exception all survive onError with their type intact. Verified to fail with the sliced form and pass with rethrow.