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

2.7: Make the timeout decorator raise an exception out of the function's s… #53799

Merged
merged 1 commit into from Mar 18, 2019

Conversation

Projects
None yet
4 participants
@mkrizek
Copy link
Contributor

mkrizek commented Mar 14, 2019

SUMMARY

…cope (#49921)

  • Revert "allow caller to deal with timeout (#49449)"

This reverts commit 6327982.

Flawed on many levels

  • Adds poor API to a public function
  • Papers over the fact that the public function is doing something bad
    by catching exceptions it cannot handle in the first place
  • Papers over the real cause of the issue which is a bug in the timeout
    decorator
  • Doesn't reraise properly
  • Catches the wrong exception

Fixes #49824
Fixes #49817

  • Make the timeout decorator properly raise an exception outside of the function's scope

signal handlers which raise exceptions will never work well because the
exception can be raised anywhere in the called code. This leads to
exception race conditions where the exceptions could end up being
hanlded by unintended pieces of the called code.

The timeout decorator was using just that idiom. It was especially bad
because the decorator syntactically occurs outside of the called code
but because of the signal handler, the exception was being raised inside
of the called code.

This change uses a thread instead of a signal to manage the timeout in
parallel to the execution of the decorated function. Since raising of
the exception happens inside of the decorator, now, instead of inside of
a signal handler, the timeout exception is raised from outside of the
called code as expected which makes reasoning about where exceptions are
to be expected intuitive again.

Fixes #43884

  • Add a common case test.

Adding an integration test driven from our unittests. Most of the time
we'll timeout in run_command which is running things in a subprocess.
Create a test for that specific case in case anything funky comes up
between threading and execve.

  • Don't use OSError-based TimeoutError as a base class

Unlike most standard exceptions, OSError has a specific parameter list
with specific meanings. Instead follow the example of other stdlib
functions, concurrent.futures and multiprocessing and define a separate
TimeoutException.

  • Add comment and docstring to point out that this is not hte Python3 TimeoutError

(cherry picked from commit bd072fe)

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/facts/timeout.py

Make the timeout decorator raise an exception out of the function's s…
…cope (#49921)

* Revert "allow caller to deal with timeout (#49449)"

This reverts commit 6327982.

Flawed on many levels

* Adds poor API to a public function
* Papers over the fact that the public function is doing something bad
  by catching exceptions it cannot handle in the first place
* Papers over the real cause of the issue which is a bug in the timeout
  decorator
* Doesn't reraise properly
* Catches the wrong exception

Fixes #49824
Fixes #49817

* Make the timeout decorator properly raise an exception outside of the function's scope

signal handlers which raise exceptions will never work well because the
exception can be raised anywhere in the called code.  This leads to
exception race conditions where the exceptions could end up being
hanlded by unintended pieces of the called code.

The timeout decorator was using just that idiom.  It was especially bad
because the decorator syntactically occurs outside of the called code
but because of the signal handler, the exception was being raised inside
of the called code.

This change uses a thread instead of a signal to manage the timeout in
parallel to the execution of the decorated function.  Since raising of
the exception happens inside of the decorator, now, instead of inside of
a signal handler, the timeout exception is raised from outside of the
called code as expected which makes reasoning about where exceptions are
to be expected intuitive again.

Fixes #43884

* Add a common case test.

Adding an integration test driven from our unittests.  Most of the time
we'll timeout in run_command which is running things in a subprocess.
Create a test for that specific case in case anything funky comes up
between threading and execve.

* Don't use OSError-based TimeoutError as a base class

Unlike most standard exceptions, OSError has a specific parameter list
with specific meanings.  Instead follow the example of other stdlib
functions, concurrent.futures and multiprocessing and define a separate
TimeoutException.

* Add comment and docstring to point out that this is not hte Python3 TimeoutError

(cherry picked from commit bd072fe)

@abadger abadger merged commit aa85959 into ansible:stable-2.7 Mar 18, 2019

1 check passed

Shippable Run 113905 status is SUCCESS.
Details
@abadger

This comment has been minimized.

Copy link
Member

abadger commented Mar 18, 2019

Merged for the 2.7.10 release.

@mkrizek mkrizek deleted the mkrizek:backport/2.7/49921 branch Mar 19, 2019

@sivel sivel removed the needs_triage label Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.