Skip to content

Conversation

@weetster
Copy link
Contributor

@weetster weetster commented Nov 13, 2025

This fix removes a call to result_handler.terminate() to ensure that the ResultHandler thread stays running and processes any task results in its queue before shutting down. The thread will terminate itself naturally after its queue is emptied.

I've added a unit test to exercise this behaviour during pool shutdown.

This fix is required for the integration tests in the following Celery pull request to pass successfully: celery/celery#9986

Fixes #434

@Nusnus Nusnus self-requested a review November 14, 2025 00:21
@auvipy auvipy requested review from auvipy and Copilot November 15, 2025 17:28
Copilot finished reviewing on behalf of auvipy November 15, 2025 17:31
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where task results could be lost during pool shutdown by ensuring the ResultHandler thread completes processing queued results before terminating.

  • Removed the premature result_handler.terminate() call in _terminate_pool() to allow natural shutdown
  • Added comprehensive test coverage for the graceful shutdown behavior with queued results
  • Added clear documentation explaining the shutdown mechanism

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
billiard/pool.py Removed result_handler.terminate() call and added documentation explaining that the ResultHandler thread will exit naturally after processing all queued results during shutdown
t/unit/test_pool.py Added simple_task helper function and test_graceful_shutdown_delivers_results test to verify that results are properly delivered when _terminate_pool() is called with pending results in the queue

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

auvipy
auvipy previously approved these changes Nov 16, 2025
Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Nusnus Nusnus merged commit 8603172 into celery:main Nov 16, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Result handler thread terminates before processing results from pool workers during shutdown

3 participants