Clean up Events for Metrics and Worker Update Threads - #5
Merged
Merged
Conversation
Contributor
|
Just did a quick test with this branch in my local docker setup and it seems to work well. |
fangpenlin
approved these changes
Apr 19, 2025
fangpenlin
left a comment
Contributor
There was a problem hiding this comment.
Tested it, it appears to be solving the shutdown hanging issue. Code also looks good to me, just minor style issue, but I can rerun black to format it. Thank you so much for contribution! 🙌
Fazel94
pushed a commit
to Fazel94/bq
that referenced
this pull request
Nov 23, 2025
Identified 8 bugs in threading implementation: CRITICAL (3): - Bug LaunchPlatform#1: Database session never closed in worker heartbeat thread - Bug LaunchPlatform#2: AttributeError when worker is None in HTTP health check - Bug LaunchPlatform#3: Session leak in HTTP request handler HIGH (2): - Bug LaunchPlatform#4: No error handling in worker heartbeat thread - Bug LaunchPlatform#5: Stale worker object in heartbeat thread MODERATE (3): - Bug LaunchPlatform#6: Race condition in metrics server shutdown - Bug LaunchPlatform#7: Potential duplicate NOTIFY when transaction is None - Bug LaunchPlatform#8: Thread join timeout may leave zombie threads Report includes detailed descriptions, reproduction steps, recommended fixes, and priority recommendations for each bug.
Fazel94
pushed a commit
to Fazel94/bq
that referenced
this pull request
Dec 30, 2025
CRITICAL FIXES (3): - Bug LaunchPlatform#1: Fixed session management in update_workers thread * Create fresh session each iteration instead of reusing * Always close session in finally block * Prevents connection pool exhaustion and stale connections - Bug LaunchPlatform#2: Fixed AttributeError in HTTP health check * Check if worker is None before accessing worker.state * Return proper error state ("NOT_FOUND") when worker is missing * Prevents HTTP server crashes - Bug LaunchPlatform#3: Fixed session leak in HTTP request handler * Close database session in finally block for all requests * Prevents connection pool exhaustion from health checks HIGH SEVERITY FIXES (2): - Bug LaunchPlatform#4: Added error handling to update_workers thread * Wrap all database operations in try/except * Log errors with full traceback * Rollback failed transactions * Continue heartbeat loop after errors (no silent death) - Bug LaunchPlatform#5: Fixed stale worker object in heartbeat thread * Refresh worker object from database each iteration * Check if worker exists (handle deletion case) * Prevents updates to detached/stale objects MODERATE FIXES (3): - Bug LaunchPlatform#6: Fixed metrics server shutdown race condition * Use threading.Event instead of callback assignment * Store server instance in thread-safe manner * Prevents race condition during early shutdown - Bug LaunchPlatform#7: Added logging for duplicate NOTIFY * Log debug message when transaction is None * Explains why deduplication is not possible * Minor performance impact, not critical - Bug LaunchPlatform#8: Fixed thread join timeout handling * Check if threads stopped after join timeout * Log errors if threads still alive * Only mark worker as SHUTDOWN if heartbeat thread stopped * Prevents zombie threads and state inconsistencies All fixes tested with syntax check and module import. Module instantiation successful.
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
3 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.
Hi there,
On exit, the worker process joins for 5s on worker update and metrics thread and then kills them.
I have added thread safe ways to signal both threads to not process the next rounds their tasks but carry on their current ones.
This would lead to a Ctrl-c or other kill signals to exit very fast and only to wait in the case the thread have ongoing code and exit when the ongoing one is finished.
For metrics thread, there is a shutdown method for the http server implemented which sets a threading.Event and the http server in its loop would check the event before processing the next request, thus would continue serving the ongoing request and would stop any other requests after the shutdown is requested.
Here is the code for the shutdown method of
WSGIServer. I just expose the method to the main thread and call it, then I would just wait for 1s to process its open requests.In the case of a user not opting in to have metrics thread, I initialized a noop function instead of shutdown, it wouldn't be called anyway.
For worker update thread, I have added a
threading.Eventinstead oftime.sleepin the worker update thread, instead of sleeping the thread would wait with a timeout of 30s on the shutdown event, if it is set, the thread would wake up and exit.If the event is set during an active working time of the thread, it would process until the end of the code and then checks it and exits.