Description of the tech debt to be addressed

Salt started on tornado before asyncio existed in the standard library. Since then we vendored it (3006.x), moved to a current tornado (3007.x), and replaced most tornado coroutines with async/await (3008.x). What is left is mostly salt.utils.asynchronous.SyncWrapper, which lets sync code call async classes.

SyncWrapper creates a private event loop per instance and blocks on it. This is the source of many open bugs: unclosed-object warnings, nested-wrapper deadlocks, task leaks, and fork/GC cleanup problems. This issue tracks removing it for 3009.x.

The hard part is not tornado itself. It is the sync callers of async code. The fix is to keep a small, well-defined sync boundary and make everything behind it async.

Related bugs

Open:

• [Bug]: 3008.3 [WARNING] unclosed SyncWrapper for cls #70355 unclosed SyncWrapper for cls
• [Bug]: Minion is warning about unclosed object/SyncWrapper/publisher client #70175 unclosed object / SyncWrapper / publisher client warnings
• [Bug]: Master MWorker deadlocks permanently in _store_job → fire_event (SyncWrapper join without timeout); wedged workers silently eat requests #69824 MWorker deadlock in _store_job -> fire_event
• [Bug]: 3008.3 minion deadlocks in zmq_ctx_term() called from a GC weakref finalizer after a master outage #70353 minion deadlock in zmq_ctx_term() from a GC finalizer

Closed:

• SyncWrapper leaks asyncio Task/Coroutine/Context per call — 18k retained over 26h on EventReturn #70169 SyncWrapper leaks asyncio tasks per call
• MWorker deadlock via nested SyncWrapper in tcp.PublishServer.publish under async fire_event #69986 MWorker deadlock via nested SyncWrapper in tcp.PublishServer.publish
• [Bug]: salt/transport/base.py:130: TransportWarning: Unclosed transport! <salt.transport.zeromq.RequestClient #69637 Unclosed transport warning

Touch the same code but SyncWrapper is not the main cause: #70178, #70344, #70071, #66435, #69988.

Current state (master, comments excluded)

SyncWrapper is created in only five places under salt/:

Other numbers:

• 84 SyncWrapper references under tests/
• 20 files under salt/ and 54 test files import tornado
• 18 @gen.coroutine: 3 in AsyncReqMessageClient (transport/zeromq.py), 15 in netapi/rest_tornado
• 54 IOLoop references and 11 run_sync calls in code
• Sync callers of async code: get_event() ~170, fire_event() ~166

Plan

Each step should be its own PR, in this order.

Step 1: Easy removals (low risk)

• Link the existing SyncWrapper bugs (see Related bugs)
• Remove the deprecated zeromq.AsyncReqMessageClient (nothing in salt/ uses it). Also removes 3 @gen.coroutine and its tornado Queue/Future use. PR: Remove deprecated zeromq.AsyncReqMessageClient #70369
• Remove the deprecated tcp.TCPReqClient, and the salt.transport.client / salt.transport.server compat modules. PR: Remove deprecated tcp.TCPReqClient and transport.client / transport.server #70375
• Remove the deprecated tcp.TCPReqServer. PR: Remove deprecated tcp.TCPReqServer #70376
• Remove the deprecated tcp.TCPPubClient and tcp.TCPPublishServer. PR: Remove deprecated tcp.TCPPubClient and tcp.TCPPublishServer #70377
• Remove the deprecated tcp.MessageClient (about 270 lines), and add unit tests for tcp.RequestClient, which had none. PR: Remove deprecated tcp.MessageClient #70378

These all had warn_until(3009, ...) and nothing in salt/ used them. Master is 3009.0, so the expired warnings also fail some unit tests in CI (RAISE_DEPRECATIONS_RUNTIME_ERRORS=1). Other expired warn_until(3009) calls (x509, etcd, vsphere, ...) are not part of this work.

Step 2: Decide the sync boundary (medium risk, design first)

• Write down which APIs stay sync (for example LocalClient, SaltEvent, the channel factory) and how they run async code (one shared background loop instead of one loop per object)
• Agree on close/cleanup rules (explicit close() or context manager vs __del__, see Remove GC-time __del__ cleanup: require explicit close / context manager #70122, Restore __del__ safety-net cleanup on SyncWrapper + PublishServer + _… #70261)

Step 3: Shared code (medium risk)

• channel/client.py (ReqChannel.factory) and fileclient.py
• utils/http.py: http.query() uses SyncWrapper around tornado's AsyncHTTPClient for the default tornado backend. About 24 callers, so decide first whether to keep the tornado HTTP backend or move to another one
• Remove IOLoop and run_sync usage (aioloop() and current_ioloop() helpers)
• cli/batch_async.py and utils/channel.py
• Update the 54 test files that import tornado

Step 4: Event bus and TCP transport (high risk)

• utils/event.py and transport/tcp.py
• Decide how get_event() / fire_event() behave when called from sync code and from inside a running loop
• Add stress tests for nested calls, fork, and shutdown
• Do this in small PRs; several related fixes are still open

Step 5: Remove SyncWrapper (after steps 1-4)

• Delete the class in salt/utils/asynchronous.py, or reduce it to the boundary helper from step 2
• Remove the __del__ safety nets that only existed for it

Out of scope unless we decide otherwise

• Replacing netapi/rest_tornado (about 2.4k lines, 15 of the @gen.coroutine). Dropping tornado as a dependency entirely would need a rewrite of salt-api.

Versions Report

N/A (tracking issue). Applies to 3008.x and master.