[3006.x] Fix nested SyncWrapper deadlock in tcp.TCPPublishServer.publish (#69986) - #69998
Merged
Merged
Conversation
When ``TCPPublishServer.publish`` was invoked from a running asyncio loop (e.g. via ``MWorker._return -> store_job -> fire_event``), the outer ``SaltEvent.pusher`` SyncWrapper's worker thread ran this coroutine, then ``self.pub_sock.send`` invoked SyncWrapper *again* -- it detected the inner thread's running io_loop, spawned yet another thread, and both deadlocked on ``threading.Thread.join()``. Detect the async context via ``asyncio.get_running_loop()`` and bypass the outer SyncWrapper. Since 3006.x's ``publish`` is sync (no ``async def``), dispatch to ``loop.create_task(...)`` as a fire-and-forget (matches the ``fire_event`` / ``spawn_callback`` precedent in ``salt/utils/event.py``). Cache a raw ``IPCMessageClient`` per running loop via ``WeakKeyDictionary`` so a fresh SyncWrapper asyncio_loop can't inherit a dead client via id() recycling. Invalidate proactively (pre-flight ``stream.closed()``) and reactively (retry once on ``salt.ext.tornado.iostream.StreamClosedError``). 3006.x-specific counterpart to 3008.x PR saltstack#69992. Fixes saltstack#69986
twangboy
requested changes
Aug 11, 2026
Before: concurrent tasks captured pub from the outer scope before taking the per-loop lock. If task A hit StreamClosedError, replaced per_loop[loop] with a healthy pub2, and released the lock, task B then acquired the lock still holding its captured pub1, tried to send on that already-closed publisher, and evicted the healthy pub2 from the cache -- cascading unnecessary reconnects under sustained concurrent publish. Re-resolve the active publisher from per_loop inside the lock so waiting tasks pick up the newly reconnected instance. Also guard the eviction path so we only pop the cache entry when it still points at the publisher we tried; a peer task's successful replacement must not be dropped. Refs review comment on PR saltstack#69998.
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.
Summary
TCPPublishServer.publishwhen in async context vialoop.create_taskIPCMessageClientper running loop viaWeakKeyDictionarysalt.ext.tornado.iostream.StreamClosedErrorboth proactively and reactively3006.x's
publishis sync (noasync def), so async dispatch is fire-and-forget matching thefire_event/spawn_callbackprecedent insalt/utils/event.py. See adaptation notes: [agents/reports/bug1-3006x-adaptation.md] (in the working branch).Fixes #69986
Test plan
salt.transport.tcpunit tests passTCPPublishServer.publishfrom inside a running asyncio loop (gap flagged by adaptation notes)Related PRs