Consolidate polling thread into background thread (PR 2b) #376
Conversation
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
This removes the redundant polling thread and consolidates all message handling into a single background thread, achieving the main goal of the refactoring. Changes: - Removed polling thread that was forwarding messages to channels - Background thread now owns the reader directly - Background thread polls messages using reader.poll_message() - Uses PendingRequests to track and match responses - Uses send_request() for non-blocking request sending - Handles events, commands, and follow-ups in single event loop Architecture: Before: Polling thread + Background thread = 2 threads After: Single background thread = 1 thread The background thread now: 1. Polls transport for messages (events/responses) 2. Processes events with on_event_nonblocking() 3. Matches responses to pending requests via PendingRequests 4. Handles commands from main thread (non-blocking check) 5. Processes follow-up requests Benefits: - 50% reduction in threads (2 -> 1) - Simpler message flow - No redundant message forwarding - Clearer separation of concerns - Foundation for future async migration All unit tests pass (7/7 in debugger crate).
This fixes the variable naming and message routing in the refactored background thread. The background thread now properly sends messages to the message channel that internals.send() receives from. Changes: - Fixed variable naming: message_tx is the sender, message_rx is the receiver - Background thread sends messages via message_tx.send() - DebuggerInternals receives via message_rx (passed during construction) - Initialize debugger AFTER background thread starts Test status: - Transport unit tests: PASS (20/20) - Transport end-to-end: PASS (1/1) - Debugger unit tests: PASS (7/7) - Debugger integration tests: HANGING (investigating) The integration tests hang which suggests a potential deadlock or timing issue that needs further debugging. The basic infrastructure works correctly.
This commit shows progress on consolidating threads but highlights that the refactoring is more complex than initially anticipated. Current state: - ✅ Removed polling thread - ✅ Single background thread forwards messages - ✅ No deadlock during initialization - ❌ Events not being processed (tests fail expecting events) The issue: - Events need to be processed by internals.on_event_nonblocking() - This converts transport::Event to state::Event and publishes to subscribers - But this requires locking internals, which we're avoiding during init - Proper event handling requires the full background thread event loop This is a working proof-of-concept but needs the complete event processing logic to pass all tests.
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.
This removes the redundant polling thread and consolidates all message handling into a single background thread, achieving the main goal of the refactoring.
Changes:
Architecture:
Before: Polling thread + Background thread = 2 threads After: Single background thread = 1 thread
The background thread now:
Benefits:
All unit tests pass (7/7 in debugger crate).