Shut down executors in hello_activity_multiprocess - #335
Open
barmoshe wants to merge 2 commits into
Open
Conversation
The sample passed a freshly constructed ProcessPoolExecutor and multiprocessing.Manager straight into the Worker and never shut either down, so the sample was teaching a resource leak. Both are now entered as context managers, outside the worker so they outlive it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two follow-ups on the previous commit: - The manager is now entered first so it is torn down last. It owns the heartbeat queue, and the SDK's heartbeat poller can still be blocked reading that queue for up to its 0.5s poll timeout after the worker exits. - create_from_multiprocessing builds its own ThreadPoolExecutor when no queue poller executor is passed, and nothing ever shuts that one down. Pass an explicit one so it is shut down with the rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
What was changed
hello_activity_multiprocess.pyconstructed aProcessPoolExecutorand amultiprocessing.Manager()inline in theWorker(...)call and never shut either down, so the sample was demonstrating a resource leak. Both are now entered as context managers outside the worker, so they outlive it and are shut down once it is done with them.Two details worth calling out:
_MultiprocessingSharedStateManager._heartbeat_processorcan still be blocked onself._heartbeat_queue.get(True, 0.5)for up to its poll timeout after the worker has exited. Shutting the manager down before the pool leaves that read on a dead connection.SharedStateManager.create_from_multiprocessingbuilds its ownThreadPoolExecutor(1)when one is not passed, and neither the shared state manager nor the worker ever shuts it down, so without this the sample would still be leaving an executor unshut. Passing our own lets it be shut down with the others.That second point is the one piece here that is arguably
sdk-python's lifetime problem rather than the sample's. If you would rather the SDK owned that executor, say so and I will drop it and keep this PR to the pool and the manager.Not included
The same inline-executor pattern (
activity_executor=ThreadPoolExecutor(5)) is in about a dozen other samples, includinghello_activity.py,hello_cron.py,hello_exception.pyandhello_mtls.py. I kept this PR to the file the issue names. Happy to do the rest as a follow-up if you want them.Issue #50 also names
hello_activity_threaded.py, which was removed in #173. Separately, the rootREADME.mdstill links that deleted file; happy to send a one-line fix for it.Testing
uv run hello/hello_activity_multiprocess.pyagainsttemporal server start-devfour times. The activity runs in a separate process, the result is correct, and the process exits in about four seconds with no shutdown traceback, confirming the added executor does not hang on shutdown.uv run poe lintclean.Checklist