Merged
Conversation
- Add client_result_promise to propagate results/errors to client_result future - Set exception on client_result_promise when connection fails - Propagate interpreter result to client_result_promise when interpreter exits - Drain all output buffers in pusher thread loop instead of one at a time - Check collection and exit state before waiting to avoid lost wakeups
Signal all notification signals (wake_up_for_output, wake_up_for_input, wake_up_for_callback, wake_up_interpreter) before joining threads in the destructor. This ensures threads waiting on these signals will wake up and check exit_when_done, preventing deadlock when the wrapper is destroyed.
- Add BindInfo struct containing port and file descriptor - Change BindResult from variant<int, string> to variant<BindInfo, string> - Query actual bound port using uv_tcp_getsockname after binding - This allows binding to port 0 to let the OS assign an available port - Update smtpserver example to use BindInfo - Update test to use port 0 and dynamically assigned port - Add ConnectionRefused test case to verify connection failure handling
Wake up all threads (interpreter, output, input, callback) when an interpreter is inserted or removed from the collection. This prevents lost wakeups where threads might be waiting on the wrong signal when a new interpreter arrives or an existing one is removed.
Lookahead operations like TerminateListIfReadAhead and TransitionLookahead store data in their context via handle_read() but return consumed=0 because they don't actually consume the bytes. The interpreter runner would see consumed=0 and assume more data is needed, but the operation is actually ready to proceed. This commit adds ready_to_evaluate() to the InputOutputOperationConcept: - Returns true if the operation has enough data to produce a result - For read operations: checks if buffer has required data or EOF reached - For write operations: always returns true (don't wait for input) - For lookahead operations: checks if conditions can be definitively matched or rejected The Continuation and Interpreter classes now expose this method, allowing the interpreter runner to check if an operation is ready even when it didn't consume any bytes.
- Add HandleBlockedResult enum with Unblocked, StillBlocked, and NeedsRetry - Join all buffer fragments in a loop instead of just two at a time - Check ready_to_evaluate() when consumed=0 to handle lookahead operations - Return NeedsRetry when buffers were joined to re-check the operation - Only wait for wake_up_interpreter if no interpreter needs retry This fixes issues where: 1. Multiple buffer fragments from one network read weren't properly joined 2. Lookahead operations that store data without consuming would block indefinitely even though they had enough data to proceed
There was a problem hiding this comment.
Pull request overview
This pull request fixes race conditions in the libuv-based network I/O implementation by addressing several concurrency issues related to thread synchronization, promise handling, and buffer management.
Key Changes:
- Introduced dynamic port assignment (port 0) for tests to avoid port conflicts and added test coverage for connection failures
- Fixed race conditions in interpreter lifecycle by adding a
ready_to_evaluate()method to determine when operations can proceed despite consuming 0 bytes - Enhanced client result promise handling to prevent hangs on connection failures by setting exceptions in all error paths
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/028-libuv-io-runner.cpp | Updated to use dynamic port assignment (port 0) and added ConnectionRefused test case |
| src/networkprotocoldsl_uv/libuvserverrunner.hpp | Added BindInfo struct to return both port and fd from bind operations |
| src/networkprotocoldsl_uv/libuvserverrunner.cpp | Implemented port detection using uv_tcp_getsockname, fixed race conditions in pusher thread and stop_accepting |
| src/networkprotocoldsl_uv/libuvclientrunner.hpp | Added client_result_promise and interpreter_result_future to properly manage result propagation |
| src/networkprotocoldsl_uv/libuvclientrunner.cpp | Set client_result_promise exceptions on all connection failure paths and propagate interpreter results |
| src/networkprotocoldsl_uv/libuvclientwrapper.cpp | Added proper signal notifications in destructor to wake threads before joining |
| src/networkprotocoldsl/operationconcepts.hpp | Added ready_to_evaluate requirement to InputOutputOperationConcept |
| src/networkprotocoldsl/operation/*.hpp/cpp | Implemented ready_to_evaluate for all I/O operations (reads, writes, lookaheads) |
| src/networkprotocoldsl/interpreterrunner.cpp | Introduced HandleBlockedResult enum and improved buffer concatenation logic with retry mechanism |
| src/networkprotocoldsl/interpretercollectionmanager.cpp | Added comprehensive signal notifications on insert/remove to prevent missed wakeups |
| src/networkprotocoldsl/interpreter.hpp | Added ready_to_evaluate method to Interpreter class |
| src/networkprotocoldsl/continuation.hpp/cpp | Implemented ready_to_evaluate for Continuation using template-based dispatch |
| examples/smtpserver/server_core.cpp | Updated to use new BindInfo struct for displaying server port information |
| scripts/debug_race.py | Added debugging script for race condition detection (contains hardcoded path) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
No description provided.