fix: Add timeout to response thread join in BrokerRequestResponse#127
Open
mo-radwan1 wants to merge 3 commits intomainfrom
Open
fix: Add timeout to response thread join in BrokerRequestResponse#127mo-radwan1 wants to merge 3 commits intomainfrom
mo-radwan1 wants to merge 3 commits intomainfrom
Conversation
The stop_component() method was calling response_thread.join() without a timeout, which could cause the process to hang indefinitely during cleanup if the response thread doesn't exit cleanly. This was causing test processes to hang during fixture teardown, requiring external timeouts to kill the process. Changes: - Added 2.0 second timeout to response_thread.join() - Added warning log if thread doesn't exit within timeout - Thread is already a daemon thread, so it will be cleaned up on process exit if it doesn't respond to the stop signal
Adds tests to verify: - Thread.join(timeout) works correctly for normal exits - Thread.join(timeout) returns after timeout for slow threads - stop_component uses join with 2.0s timeout - Warning is logged when thread doesn't exit in time - Daemon threads don't prevent process exit
Hugo-Pare
approved these changes
Feb 3, 2026
Add 2-second timeouts to thread.join() calls in multiple locations: - TimerManager.stop(): timer thread join - CacheService.stop(): expiry thread join - ComponentBase.run(): monitoring and connection status thread joins - Flow.wait_for_threads(): flow thread joins All these threads are daemon threads, so they won't prevent process exit if the join times out. Added warning logs for TimerManager and CacheService when threads don't exit within the timeout. This completes the fix started in the previous commit that added timeout to BrokerRequestResponse.stop_component().
|
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
2 participants
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.
🧩 Complexity Level: 🟢 Low
⏱️ Estimated Review Time: 5 minutes
What is the purpose of this change?
Fix a process hang issue during cleanup when the response thread in
BrokerRequestResponse.stop_component()doesn't exit cleanly.The
response_thread.join()call had no timeout, which could cause the process to hang indefinitely during component cleanup. This was observed during test fixture teardown in downstream projects, requiring external timeouts to kill the test process.How is this accomplished?
response_thread.join(timeout=2.0)daemon=True, so it will be cleaned up on process exit even if it doesn't respond to the stop signalThe 2.0 second timeout is chosen to be longer than the 1.0 second
receive_messagetimeout inhandle_responses(), giving the thread enough time to notice the stop signal and exit gracefully.Anything reviewers should focus on/be aware of?
solace_ai_connector.py:698usesthread.join(timeout=1.0))join()call from blocking indefinitely