fix race condition and KeyError exception in executor #9335
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
|
Note that 1.8.x is also affected. I'd like to maybe see this get cherry-picked to that branch as our CI is currently running into this issue. |
Secrus
reviewed
Apr 29, 2024
vfazio
force-pushed
the
vfazio-fix-race-condition
branch
2 times, most recently
from
2668578
to
7e9fb9d
Compare
Secrus
approved these changes
Apr 29, 2024
Previously, it was possible that `Executor.supports_fancy_output` would flip-flop between True and False if a thread was updating a section. This could lead to a crash when an operation got different reponses as it made progress. `Executor.supports_fancy_output` reflects the value of the underlying cleo Formatter used by the Output object. The Formatter is shared by any SectionOutputs derived by that Output object. If a thread (tA) is in the middle of `Formatter.remove_format` while updating a section, the flag to show decorator support is temporarily toggled off and then restored. This opens a window where another thread could get an incorrect answer when querying `supports_fancy_output`. If a parallel thread (tB) queries `supports_fancy_output` and sees it is False, the operation will not get added to the Executor's _sections dictionary. If tB's operation progresses after tA has restored the decorator value and attempts to write out progress information it will call `Executor._write`, see that `supports_fancy_output` is now True and attempt to find the operation in the _sections dictionary, however there will not be an entry for that operation due to the earlier query that returned False. This causes tB to throw a KeyError and causes the install to shutdown. Now, the Executor queries and caches whether the Output is decorated during init. This value is used in `supports_fancy_output` so as to not be affected by changes to the underlying Formatter object during section updates. Signed-off-by: Vincent Fazio <[email protected]>
vfazio
force-pushed
the
vfazio-fix-race-condition
branch
from
7e9fb9d
to
4454153
Compare
radoering
added
impact/backport
poetry-bot bot
pushed a commit
that referenced
this pull request
Apr 30, 2024
Previously, it was possible that `Executor.supports_fancy_output` would flip-flop between True and False if a thread was updating a section. This could lead to a crash when an operation got different reponses as it made progress. `Executor.supports_fancy_output` reflects the value of the underlying cleo Formatter used by the Output object. The Formatter is shared by any SectionOutputs derived by that Output object. If a thread (tA) is in the middle of `Formatter.remove_format` while updating a section, the flag to show decorator support is temporarily toggled off and then restored. This opens a window where another thread could get an incorrect answer when querying `supports_fancy_output`. If a parallel thread (tB) queries `supports_fancy_output` and sees it is False, the operation will not get added to the Executor's _sections dictionary. If tB's operation progresses after tA has restored the decorator value and attempts to write out progress information it will call `Executor._write`, see that `supports_fancy_output` is now True and attempt to find the operation in the _sections dictionary, however there will not be an entry for that operation due to the earlier query that returned False. This causes tB to throw a KeyError and causes the install to shutdown. Now, the Executor queries and caches whether the Output is decorated during init. This value is used in `supports_fancy_output` so as to not be affected by changes to the underlying Formatter object during section updates. Signed-off-by: Vincent Fazio <[email protected]> (cherry picked from commit acaf9c8)
radoering
pushed a commit
that referenced
this pull request
Apr 30, 2024
Previously, it was possible that `Executor.supports_fancy_output` would flip-flop between True and False if a thread was updating a section. This could lead to a crash when an operation got different reponses as it made progress. `Executor.supports_fancy_output` reflects the value of the underlying cleo Formatter used by the Output object. The Formatter is shared by any SectionOutputs derived by that Output object. If a thread (tA) is in the middle of `Formatter.remove_format` while updating a section, the flag to show decorator support is temporarily toggled off and then restored. This opens a window where another thread could get an incorrect answer when querying `supports_fancy_output`. If a parallel thread (tB) queries `supports_fancy_output` and sees it is False, the operation will not get added to the Executor's _sections dictionary. If tB's operation progresses after tA has restored the decorator value and attempts to write out progress information it will call `Executor._write`, see that `supports_fancy_output` is now True and attempt to find the operation in the _sections dictionary, however there will not be an entry for that operation due to the earlier query that returned False. This causes tB to throw a KeyError and causes the install to shutdown. Now, the Executor queries and caches whether the Output is decorated during init. This value is used in `supports_fancy_output` so as to not be affected by changes to the underlying Formatter object during section updates. Signed-off-by: Vincent Fazio <[email protected]> (cherry picked from commit acaf9c8)
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Previously, it was possible that Executor.supports_fancy_output would flip-flop between True and False if a thread was updating a section. This could lead to crashes when an operation got different reponses as it made progress.
Executor.supports_fancy_output reflects the value of the underlying cleo Formatter used by the Output object. The Formatter is shared by any SectionOutputs derived by that Output object.
If a thread (tA) is in the middle of Formatter.remove_format, the flag to show decorator support is temporarily toggled off and then restored, which opens a window of time where another thread could get an "incorrect" answer when queried via
supports_fancy_output.If a parallel thread (tB) queries
supports_fancy_outputand sees it's False, the operation would not get added to the Executor's _sections dictionary. If tB's operation progressed after tA has restored the decorator value and attempts to write out progress information it will callExecutor._write, see thatsupports_fancy_outputis now True and attempt to find the operation in the _sections dictionary, however there will not be an entry for that operation due to the earlier query that returned False.This causes tB to throw a KeyError and causes the install to shutdown.
Now, the Executor queries and caches whether the Output is decorated during init. This value is used in
supports_fancy_outputso as to not be affected by changes to the underlying Formatter object during section updates.Pull Request Check List
Resolves: #9334