-
Notifications
You must be signed in to change notification settings - Fork 0
Add timeouts to ThreadPoolExecutor result() calls to prevent indefinite hangs #169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… indefinite hangs Co-authored-by: redreceipt <[email protected]>
Co-authored-by: redreceipt <[email protected]>
Co-authored-by: redreceipt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reviews_future = executor.submit(merged_prs_by_reviewer, days) | ||
| authored_prs_future = executor.submit(merged_prs_by_author, days) | ||
|
|
||
| created_priority_bugs = created_priority_future.result() | ||
| open_priority_bugs = open_priority_future.result() | ||
| created_priority_bugs = get_future_result_with_timeout(created_priority_future, []) | ||
| open_priority_bugs = get_future_result_with_timeout(open_priority_future, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index future timeouts never take effect
These get_future_result_with_timeout(..., timeout=INDEX_FUTURE_TIMEOUT) calls run only after the surrounding with ThreadPoolExecutor(...) block has exited. The executor context manager calls shutdown(wait=True) before reaching this point, which blocks until all submitted futures finish. As a result, the new 10s timeouts can never fire and a hung get_* call will still hang _build_index_context, so the change does not actually prevent the indefinite waits it is meant to address.
Useful? React with 👍 / 👎.
ThreadPoolExecutor
.result()calls were blocking indefinitely when background tasks failed or took too long, causing the application to hang.Changes
get_future_result_with_timeout()wraps.result()calls with timeout and returns default values on timeoutINDEX_FUTURE_TIMEOUT = 10for concurrent index operations, distinguished fromEXECUTOR_TIMEOUT_SECONDS = 30for individual route operationsMinor: Removed redundant "for debugging" from logging comments.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.