python: release the GIL around the operations that block (ibx#271) - #391
Open
userFRM wants to merge 1 commit into
Open
python: release the GIL around the operations that block (ibx#271)#391userFRM wants to merge 1 commit into
userFRM wants to merge 1 commit into
Conversation
The Python EClient held the GIL across every operation that can block for an unbounded time: joining the engine thread in disconnect() and Drop, sending on the bounded(64) control channel, and waiting on the registration reply channel used by find_or_register_instrument, register_mkt_data, and register_tbt. connect() and run() already released the GIL around their own blocking calls, the gateway login and wait_for_data; the rest of the surface did not, so a stalled engine thread, the wedge deepentropy#254 describes one way to get there, froze the interpreter instead of just blocking its own caller, taking every unrelated Python thread down with it. disconnect() and Drop now detach around the thread join. Drop has no Python<'_> token to work with, so it mints one through Python::try_attach rather than Python::attach: dealloc can also run during interpreter shutdown, and since EClient.wrapper commonly points back at the object embedding it (the App(EWrapper, EClient) pattern passes self as the wrapper), the cyclic GC, not just refcounting, can be what calls drop, and attaching is unsound in the shutdown case. When try_attach declines, drop falls back to the plain join it used before this change rather than lose the handle. find_or_register_instrument (used by place_order), register_mkt_data (req_mkt_data), and register_tbt (req_tick_by_tick_data) each wait on a bounded(1) reply channel for up to REGISTRATION_TIMEOUT to hear back from the hot loop. Those call sites now copy the contract fields they need into owned values before detaching for the round trip, so a slow reply stalls the caller rather than the interpreter. client_core.rs itself is untouched: it is shared with the plain Rust API, which has no GIL to release, so the fix lives entirely on the Python call sites that wrap it. Every control_tx.send across orders.rs, market_data.rs, reference.rs, and account.rs now goes through EClient::send_control. The channel is bounded(64) on purpose, applying backpressure to a caller that outruns the hot loop, so the send itself stays a normal blocking send rather than becoming a probe that fails as soon as the queue is momentarily full. What changes is that the wait no longer holds the GIL: send_control builds the command while still attached, then moves it into a detached closure around the send, so a caller stalled behind a full queue blocks only itself instead of every Python thread. A SendError (the channel gone, not merely full) still surfaces as the same "Engine stopped" PyRuntimeError callers saw before this change. cargo check --offline --lib --features python is clean. cargo test --lib --features python does not link in this environment (ibx#381) and could not be run; the pytest suite under tests/python needs the same built extension module and could not be exercised either. The full gate suite passes: check --lib, check --lib --features python, check --bins, check --examples, every tests/*.rs check including the ib_paper_compat baseline diff, and cargo test --lib, with the two config::expiry_tests tzdata failures that are expected in this environment. Closes deepentropy#271.
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
1 participant
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.
Problem
The Python layer held the GIL across operations that block. A thread join on
disconnect()and onDrop, the registration waits that can sit for up to five seconds, and every control-channel send: each one blocked with the interpreter locked, so one stalled call froze every other Python thread in the process.What this changes
The GIL is released around the wait and reacquired after — the blocking operation is what moves inside the detach, and nothing Python-backed crosses that boundary. Commands are built while attached, so only an owned
ControlCommandand a channel reference travel with the closure.The sends stay blocking. The channel is bounded, so a burst applies backpressure and then succeeds, which is the behaviour callers already have; what was wrong was holding the GIL while waiting, not the waiting itself.
Dropmints its token withPython::try_attachrather thanPython::attach. Dealloc can run during interpreter shutdown, and the commonApp(EWrapper, EClient)shape means the cyclic collector rather than refcounting may be what calls it. Where the token cannot be had, the join still happens — the handle is never dropped unjoined.ClientCoreis untouched. It is shared with the plain Rust API, which has no GIL to release, so the fix stays on the Python side that wraps it.Verification
cargo check --offline --lib --features pythonis clean, and the full gate passes.Not run:
cargo test --lib --features pythonand the pytest suite, neither of which links in this tree (#381). The unblocking behaviour and thetry_attachshutdown path are established by reading pyo3 0.29'sUngilbounds anddetach/try_attachcontracts against this repo's existing usage inconnect()andrun(), rather than by exercising an interpreter.Closes #271.