Timer event order pollution #1851
Merged
Conversation
This file contains 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.
This PR fixes a bug (found by https://groups.google.com/g/mpf-users/c/JtZxG7oOceQ/m/P3htggdmAwAJ) where the order of a Timer's
control_eventscould cause a crash.The Problem
There has been a longstanding (7+ years) flaw in the timer control event code where the event handler kwargs were a singleton dictionary, so each event handler would alter/amend the same dictionary and attach it as kwargs. This oversight slipped by largely unnoticed until earlier this year, when the timer code was revamped to pass more event kwargs through (to support dynamic placeholder values for tick value, tick interval, et cetera., see #1761).
Even then, this oversight slipped through because the timer kwarg pollution is dependent on the order in which the timer's
control_eventsare listed. Only if a control event with avaluecomes before a reset/restart will the former event's timer_value kwarg be polluted into the reset/restart handler, causing a duplicate argument error.The Fix
This PR simply moves the
kwargs = {}declaration to be inside the control event iteration block, so each control event starts with a fresh, empty, unpolluted kwarg dictionary. With this change, the order of control events doesn't matter and pollution is avoided.