Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix race condition and KeyError exception in executor #9335

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

vfazio
Copy link
Contributor

@vfazio vfazio commented Apr 24, 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 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_output and 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 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.

Pull Request Check List

Resolves: #9334

  • Added tests for changed code.
  • Updated documentation for changed code.

@vfazio
Copy link
Contributor Author

vfazio commented Apr 24, 2024

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.

@vfazio vfazio force-pushed the vfazio-fix-race-condition branch 2 times, most recently from 2668578 to 7e9fb9d Compare April 29, 2024 13:13
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 <vfazio@gmail.com>
@radoering radoering added impact/backport Requires backport to stable branch backport/1.8 Backport a PR to 1.8 through an automated PR labels Apr 30, 2024
@radoering radoering merged commit acaf9c8 into python-poetry:main Apr 30, 2024
62 checks passed
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 <vfazio@gmail.com>
(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 <vfazio@gmail.com>
(cherry picked from commit acaf9c8)
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.8 Backport a PR to 1.8 through an automated PR impact/backport Requires backport to stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

poetry install crashes due to a race condition and unreported KeyError
3 participants