Repository navigation
fix: keep event order, measure delays on the monotonic clock, cancel by marking - #665
Open
JMerrill-Fairness wants to merge 8 commits into
Open
JMerrill-Fairness wants to merge 8 commits into
JMerrill-Fairness wants to merge 8 commits into
Conversation
Both queues are PriorityQueues ordered by TriggerData's execution_time alone, so triggers with equal times came out in heap order rather than the order they were put. Two events stamped alike by a coarse clock could run out of order; twenty raised in one callback with one stamp ran as 0, 2, 6, 14, 19, 18, ... TriggerData now numbers each trigger as it is created and compares that number after execution_time, so triggers due together keep their order. The new tests stamp every trigger with one time and check the order on both engines and both queues. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Triggers were stamped with time.time() plus their delay and held until time.time() reached the stamp, so a step of the wall clock moved every due time: stepped back, it held even an undelayed event until the clock caught up; stepped forward, it ran delayed events early. Stamps and the due checks in both engines now use time.monotonic(). The new delayed-event test drives a fake monotonic clock in all three modules, so a stamp on one clock checked against another would hold the event and fail. The ordering tests now fix the monotonic clock instead of the wall clock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EventQueue.remove() filtered the PriorityQueue's underlying list. A filtered heap is not always a heap, so the triggers left could come out of due order: of ten delayed events due from 10 to 90 ms, cancelling the first ran the event due at 50 ms before the one due at 40 ms. The trigger's future was dropped with it, so on the async engine a coroutine awaiting the cancelled event's send() was never answered. cancel_event() now marks each trigger sent with the send_id as cancelled and answers its future with None. The triggers stay where they are, so the heap is never rearranged from outside, and is_empty() drops a cancelled trigger when it reaches the head, before the processing loop could check whether it is due. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tests drawn from the change's own code rather than its description: - cancel_event() called from a callback, on both engines, and a caller that stops waiting, before or while it awaits, leaving its event cancellable; - a send_id matched exactly: empty, whitespace, line endings, NUL, case, a zero-width space, a byte-order mark, composed against decomposed accents, and ids up to 2048 characters, none of which cancels an event sent without one; - a cancel with nothing queued cancels nothing sent later, two ids cancelled in turn cancel each, and a cancelled trigger is marked rather than removed; - the order test cancels the first, a middle and the last event due, and the last one queued; - no trigger is stamped or checked on the wall clock, the engines read time.monotonic, and a delay in milliseconds comes due that many thousandths of a second later, for none, 0, 1, 50 and -50; - events due together keep their order for 0, 1, 2 and 20 of them. Each was checked against mutants of the code it covers: 31, every one failed by at least one test without waiting for a timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SonarCloud's S9083 asks for `@pytest.fixture` without parentheses, and this project's ruff configuration (fixture-parentheses = true) asks for them. The two clock fixtures are now `autouse=True`, which both accept: every test in each module runs on the substituted clock, so none reads the real one, and a test that moves the clock asks for it by name. The send_ids that held a zero-width space, a byte-order mark and accents as literal characters are escapes again, so the file is ASCII. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A delayed event queued from a callback, while the processing loop runs, comes due on a clock that moves 10 ms with every reading, on both engines. A coroutine that stops awaiting a delayed event, before or while it awaits, does not stop the event, which fires in due order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No `type: ignore`, `noqa`, `pragma: no cover` or `pragma: no branch` is left in base.py, sync.py, async_.py or statemachine.py. - BaseEngine declared select_transitions, select_eventless_transitions, _select_transitions, _first_transition_that_matches, microstep and _exit_states as synchronous, and AsyncEngine overrode them with coroutines, which mypy rejected. They now belong to SyncEngine, and BaseEngine keeps what both engines share. - Each processing loop is split into its macrostep, the external events and one external event, and add_descendant_states_to_enter into its history, region and initial-state parts. - Removed as unreachable: BaseEngine.empty and pop, which nothing called; the _run_microstep wrappers, since microstep already sends every error through _handle_error; the second pass over the internal queue, since the macrostep empties it and spawning invocations puts events only on the external queue; and the checks for an exited state being None. - is_in_final_state takes the parallel case first, so an atomic state needs no branch of its own. - StateChart: a cast for the default Model, vars(self).update, an assert that narrows initial_state, and _repr_svg_ through the diagram formatter, which renders SVG the same way. New tests cover the two lines the pragmas hid: stop() still stops the engine when cancelling invocations fails, and a transition into a parallel state and a state inside one of its regions enters that state and not the region's initial one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SyncEngine.processing_loop took a caller_future it never used: only AsyncEngine.put attaches a future to an event, so the sync engine was always handed None. StateChart._processing_loop now passes the future to an AsyncEngine and calls the sync loop without one, and the sync loop no longer takes it (SonarCloud python:S1172). The new SCXML test marks its module with `pytestmark` rather than `@pytest.mark.scxml()`, which this project's ruff configuration (mark-parentheses = true) and SonarCloud's python:S9083 disagree on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This branch has not been deployed
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.



Three fixes to how the engines order, time and cancel events, and a refactor that removes every suppression from the modules they touch. Each is its own commit, and they can be split into separate PRs if you prefer.
Events due at the same moment kept their order only by chance
Both queues are
PriorityQueues ordered byTriggerData.execution_timealone, so triggers with equal times come out in heap order, not the order they were put. A clock coarse enough to stamp two events alike runs them out of order. With every trigger stamped alike, twenty events raised in one callback ran as0, 2, 6, 14, 19, 18, ..., on both engines and both queues.TriggerDatanow numbers each trigger as it is created (itertools.count) and compares that number afterexecution_time, so triggers due together keep their order.Delays were measured on the wall clock
Triggers were stamped with
time.time()plus their delay and held untiltime.time()reached the stamp, so a step of the wall clock moved every due time. Stepped back by a time sync, it held even an undelayed event until the clock caught up. Stepped forward, it ran delayed events early. Stamps and the due checks in both engines now usetime.monotonic().Cancelling a delayed event could disorder the others and strand its caller
EventQueue.remove()filtered the list behind the heap, and a filtered heap is not always a heap. Of ten delayed events due from 10 to 90 ms, cancelling the first ran the event due at 50 ms before the one due at 40 ms. The trigger's future was dropped with it, so on the async engine a coroutine awaiting the cancelled event'ssend()was never answered. Both happen on 3.2.1.cancel_event()now marks each trigger sent with thesend_idas cancelled, in a newTriggerData.cancelledfield, and answers its future withNone. The triggers stay where they are, so nothing rearranges the heap from outsidePriorityQueue, andEventQueue.is_empty()drops cancelled triggers from the head, so the processing loop never checks whether one is due.EventQueue.remove()is renamedcancel().The suppressions in the touched modules
No
type: ignore,noqa,pragma: no coverorpragma: no branchis left inengines/base.py,engines/sync.py,engines/async_.pyorstatemachine.py. Each was replaced by fixing what it covered:BaseEnginedeclaredselect_transitions,select_eventless_transitions,_select_transitions,_first_transition_that_matches,microstepand_exit_statesas synchronous, andAsyncEngineoverrode them with coroutines, which is what the fourtype: ignore[override]hid. They now belong toSyncEngine, andBaseEnginekeeps what both engines share.add_descendant_states_to_enterinto its history, region and initial-state parts, which takes every function under the complexity limit.BaseEngine.empty()andpop(), which nothing called; the_run_microstep()wrappers, sincemicrostep()already sends every error through_handle_error(); the second pass over the internal queue, since the macrostep empties it and spawning invocations puts events only on the external queue; and the checks for an exited state beingNone.SyncEngine.processing_loop()no longer takes acaller_futureit never used. OnlyAsyncEngine.put()attaches a future to an event, soStateChart._processing_loop()now passes one to the async engine alone.is_in_final_state()takes the parallel case first, so an atomic state needs no branch of its own.StateChart: acastfor the defaultModel,vars(self).update(), anassertthat narrowsinitial_state(asfactory.pydoes), and_repr_svg_()through the diagram formatter, which renders SVG the same way.Two lines the pragmas hid now have tests:
stop()still stops the engine when cancelling invocations fails, and a transition into a parallel state and a state inside one of its regions enters that state and not the region's initial one.Tests
tests/test_event_ordering.pystamps every trigger with one time (by patchingstatemachine.event_data.monotonic) and checks the order on both engines and both queues, for 0, 1, 2 and 20 events. It failed before its fix.tests/test_cancel_event.pyruns every test on a clock that moves only when the test moves it.send_idcancels every event sent with it and nothing else. It is matched exactly: empty, whitespace, line endings, NUL, case, a zero-width space, a byte-order mark, composed against decomposed accents, and ids up to 2048 characters. It never cancels an event sent without one, and a cancel with nothing queued cancels nothing sent later.cancel_event()works from inside a callback, and a cancelled trigger is marked rather than removed.None, whether it is cancelled once or twice, and a caller that stops waiting, before or while it awaits, leaves its event cancellable.tests/test_statechart_delayed.pychecks that no trigger is stamped or checked on the wall clock (readingtime.timefails the test), that the engines readtime.monotonic, and that a delay in milliseconds comes due that many thousandths of a second later, for no delay, 0, 1, 50 and -50.tests/test_statechart_delayed.pyalso queues a delayed event from a callback, on a clock that moves 10 ms with every reading, and checks that a coroutine that stops awaiting a delayed event, before or while it awaits, does not stop it firing in due order.tests/test_invoke.py::TestEngineStopCleanupandtests/scxml/test_parallel_regions.pycover the two lines the pragmas hid.--cov-fail-under=100, along with ruff, mypy and pyright, through the pre-commit hooks.Release notes are in
docs/releases/3.2.2.md.🤖 Generated with Claude Code