fix(Net): reactor-based http server not response when requests with body #5227
fix(Net): reactor-based http server not response when requests with body #5227matejk merged 7 commits intopocoproject:mainfrom
Conversation
matejk
left a comment
There was a problem hiding this comment.
Comprehensive Review
The PR fixes three real bugs:
- Body reading —
read()override is needed and the approach is correct - CTRL+C hang —
_stoppedguard + worker reactor stop in destructor fixes the shutdown hang - Include guard — correct, follows POCO convention
However, there are several issues to address: a missing newline at EOF, code style violations (POCO uses Allman brace style), an incomplete shutdown sequence, and a questionable use of chunked encoding for empty error responses. See inline comments for details.
Missing test coverage
There is no test that exercises the body-reading path (e.g., a POST request with body via the reactor server). The only test change adds reactor->stop() to an existing test. A dedicated test for body reading would significantly increase confidence in the fix.
| worker->stop(); | ||
| } | ||
| if(_threadPool) { | ||
| _threadPool->joinAll(); |
There was a problem hiding this comment.
| _threadPool->joinAll(); | |
| void TCPReactorAcceptor::stop() | |
| { | |
| for (auto& worker : _wokerReactors) | |
| { | |
| worker->stop(); | |
| } | |
| if (_threadPool) | |
| { | |
| _threadPool->joinAll(); | |
| } | |
| } |
Code style: POCO uses Allman brace style (opening brace on its own line) and requires a space before ( in control statements (for (, if (). The current code uses K&R/attached style which is inconsistent with the rest of the codebase.
Also, stop() is not idempotent — it can be called explicitly and then again from the destructor. Consider adding an std::atomic<bool> _stopped guard (like TCPReactorServer does) for consistency and safety.
|
|
||
| void TCPReactorServer::stop() | ||
| { | ||
| if (_stopped.exchange(true)) |
There was a problem hiding this comment.
The _stopped guard is good and fixes the CTRL+C double-stop issue.
However, stop() only stops the main acceptor reactors — the worker reactors inside each TCPReactorAcceptor are only stopped later when the acceptors are destroyed in the TCPReactorServer destructor. Between stop() returning and destruction, worker threads are still running. Consider also stopping acceptors explicitly:
| if (_stopped.exchange(true)) | |
| if (_stopped.exchange(true)) | |
| { | |
| return; | |
| } | |
| for (auto& acceptor : _acceptors) | |
| { | |
| acceptor->stop(); | |
| } |
This ensures all threads (both acceptor and worker) are fully stopped when stop() returns, rather than relying on eventual destructor cleanup.
| }; | ||
|
|
||
|
|
||
| class BodyEchoRequestHandler: public HTTPRequestHandler |
There was a problem hiding this comment.
Nit: the empty constructor body can be replaced with = default:
| class BodyEchoRequestHandler: public HTTPRequestHandler | |
| class BodyEchoRequestHandler: public HTTPRequestHandler | |
| /// Echo the request body back to the client. | |
| { | |
| public: | |
| BodyEchoRequestHandler() = default; |
|
@sky92zwq, can you take a look at the code review comments? IMO it makes sense merging this PR into next patch release. |
@matejk OK. I’ve been quite busy these past two weeks, but I’ll try to address the review comments over the weekend. I’m also working on optimizing performance for a large number of concurrent connections. |
|
Release for 1.15.1 is planned for next week. Shall we merge this PR and put performance optimisations to 1.15.2 or even 1.16? |
SGTM. Merging this now and handling the performance optimizations later makes sense. |
matejk
left a comment
There was a problem hiding this comment.
Thanks for addressing most of the feedback! Most issues are now fixed. Three remaining items below.
| std::shared_ptr<ThreadPool> _threadPool; | ||
| RecvMessageCallback _recvMessageCallback; | ||
| TCPServerParams::Ptr _pParams; | ||
| std::atomic<bool> _stopped; |
There was a problem hiding this comment.
Bug: _stopped is not initialized. std::atomic<bool> has a trivial default constructor that leaves the value indeterminate in C++17 and earlier. If _stopped happens to start as true, stop() will return immediately without stopping anything.
Should be:
| std::atomic<bool> _stopped; | |
| std::atomic<bool> _stopped{false}; |
(Note: TCPReactorServer correctly initializes _stopped(false) in its constructor initializer list, but TCPReactorAcceptor does not.)
Net/src/TCPReactorAcceptor.cpp
Outdated
| stop(); | ||
| } | ||
|
|
||
| void TCPReactorAcceptor::stop() { |
There was a problem hiding this comment.
Minor style nits still remaining:
- Function definition brace should be on its own line (Allman style)
- Missing space before
(infor((line 43) andif((line 47)
| void TCPReactorAcceptor::stop() { | |
| void TCPReactorAcceptor::stop() | |
| { |
Net/src/TCPReactorServer.cpp
Outdated
| _reactors(pParams->getAcceptorNum()), | ||
| _pParams(pParams), | ||
| _port(port) | ||
| : _threadPool("TCPRA", pParams->getAcceptorNum()), |
There was a problem hiding this comment.
The initializer list indentation changed from tab-indented (POCO convention) to no indentation:
// Before (POCO style)
\t: _threadPool(...),
\t _reactors(...),
// Now
: _threadPool(...),
_reactors(...),This is inconsistent with the rest of the codebase. The same applies to HTTPReactorServer.cpp.
…ody (#5227) * fix: reactor-based http server, send error when exp, recv req body * add POST interface in reactor server * fix: exit stuck * fix: reactor-based http server redefination * fix: tsan about socket reactor testConcurrentHandlerRemoval * address review, add unit tests * initialize _stopped, and coding style
Some problems fixed PR :
CTRL + Ccorrectly.HTTPReactorServer.h