Harden transaction processing pool shutdown - #9762
Open
jhonabreul wants to merge 8 commits into
Open
Conversation
Bound the pool's dispose so a request handler pinned in a blocking call cannot hold up the engine shutdown, and report the requests that were dropped without being processed: - Join the worker threads against a single shared deadline instead of a full timeout per thread, and interrupt the ones that do not stop - Cancel up front so workers exit instead of draining a backlog that should not be sent to the brokerage while shutting down - Drop the requests still queued or parked once the workers are stopped, reporting each one so the transaction handler invalidates its order before the final results are sent, instead of leaving it as New
Address review feedback: instead of the pool draining its queues from the outside and reporting each request through a callback, the handler sets a flag at exit and the requests still queued flow through the normal request handling, which invalidates them instead of processing: - Parked per-order requests are re-enqueued onto the shared queue at dispose, and the surviving workers drain the backlog naturally - If every worker was stuck, a background drainer thread chews the backlog with a bounded join, so Dispose cannot block even when invalidating an order itself blocks - Reverts the IBusyCollection.TryTake addition, no longer needed
A cancel issued after the last pump, e.g. from OnEndOfAlgorithm, is still queued when the handler exits: assert it gets the "never sent to the brokerage" error response instead of being silently discarded, the order is not canceled and no extra order events are emitted.
Both join loops now handle the disposing thread being interrupted while waiting, so an exception cannot abort the engine shutdown mid-way, and a trace at the end of Dispose records that it completed.
One interrupted join no longer skips joining the remaining stuck workers. Interrupting a started thread cannot throw, so the interrupt loop needs no guard.
A slow handler holds the order event lock, and for Python the GIL, stalling the other transaction threads and the order status processing. Handlers taking over 5 seconds get the user warned, once; after that the handler is no longer measured.
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.
Description
Disposing the
OrderRequestProcessingPoolcould take 2 minutes per worker thread and silently discard queued order requests. A brokerage request that blocks indefinitely (e.g. a native API wait for a response that never arrives) pins every worker: shutdown then takes2 min x N threads, and the orders whose requests never ran stayNewin the final results, with no event explaining why.The fix:
BrokerageTransactionHandler.Exit()sets a flag before disposing the pool; the normal request handling checks it and invalidates the request instead of processing it (OrderStatus.Invalid, "the order was never sent to the brokerage").Transactions.Exit()runs before the final results are sent, so the events are recorded.OrderRequestProcessingPool.Disposere-enqueues parked per-order requests onto the shared queue and completes adding without cancelling the token: surviving workers drain the backlog through the normal loop, where the flag turns each request into an invalidation.ShutdownTimeout) instead of a full timeout per thread; stragglers getThread.Interrupt(), which frees even a native wait — the interrupted in-flight submit is invalidated by the existingPlaceOrderexception handling.Disposestays bounded even when invalidating an order itself blocks.DraintreatsOperationCanceledException/ThreadInterruptedExceptionduring shutdown as a quiet exit (when (_shuttingDown)), so a normal stop cannot report an algorithm runtime error. Outside shutdown they still reachonError.OnOrderEventhandler runs slow, since order processing is blocked while it runs.Related Issue
N/A — observed in live trading: a stop took
2 min x 10 threadswhile the affected orders stayedNewin the final results.Motivation and Context
Engine shutdown must be bounded regardless of brokerage behavior, and the final results must reflect orders that were never sent.
Requires Documentation Change
No.
How Has This Been Tested?
OrderRequestProcessingPoolTests(4 tests): queued and parked requests all reach the request handler exactly once during a bounded dispose; a handler stuck in a blocking wait is interrupted and the backlog is drained on the dedicated drainer thread; an idle pool disposes fast and quiet; the synchronous pool variant processes pending requests.BrokerageTransactionHandlerTestssuite: 196 passed, 0 failed.Invalidevent in the final results, and the process exits cleanly.Types of changes
Checklist:
bug-<issue#>-<description>orfeature-<issue#>-<description>