Skip to content

bpo-46711: increase timeout for test_logging::test_post_fork_child_no_deadlock#31274

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.

Sign up for GitHub

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

Closed
sobolevn wants to merge 3 commits into from
Closed

bpo-46711: increase timeout for test_logging::test_post_fork_child_no_deadlock#31274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae6e18a
bpo-46711: increase timeout for `test_logging::test_post_fork_child_n…
sobolevn Feb 11, 2022
c7c20a4
Use `timeout` in `support.wait_process`
sobolevn Feb 11, 2022
b067c74
Add info about timeout
sobolevn Feb 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Lib/test/support/__init__.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2056,7 +2056,7 @@ def wait_process(pid, *, exitcode, timeout=None):
# process is still running

dt = time.monotonic() - t0
if dt > SHORT_TIMEOUT:
if dt > timeout:
try:
os.kill(pid, signal.SIGKILL)
os.waitpid(pid, 0)
Expand All@@ -2065,7 +2065,8 @@ def wait_process(pid, *, exitcode, timeout=None):
pass

raise AssertionError(f"process {pid} is still running "
f"after {dt:.1f} seconds")
f"after {dt:.1f} seconds, "
f"timeout is {timeout} seconds")

sleep = min(sleep * 2, max_sleep)
time.sleep(sleep)
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_logging.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -747,7 +747,7 @@ def lock_holder_thread_fn():
fork_happened__release_locks_and_end_thread.set()
lock_holder_thread.join()

support.wait_process(pid, exitcode=0)
support.wait_process(pid, exitcode=0, timeout=support.LONG_TIMEOUT)

@vstinnervstinnerFeb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please change the default timeout in wait_process() instead?

@sobolevnsobolevnFeb 11, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might have some consequences 🤔

Right now SHORT_TIMEOUT is just 30 seconds, while LONG_TIMEOUT is 5 minutes.
It is a 10x increase.

Secondly, current docs state (https://docs.python.org/3/library/test.html#test.support.SHORT_TIMEOUT):

If a test using SHORT_TIMEOUT starts to fail randomly on slow buildbots, use LONG_TIMEOUT instead.

Initially I've followed this recommendation.

Do we have other tests that fail due to this timeout? Is it a global problem? You totally have more data than me on this problem.

Lastly, regrtest has --timeout, which we can change for slower runners.

If you think that we still should change the default, I have several questions:

  1. I guess we need to add .. versionchanged:: 3.11 to wait_process docs and change the default value there
  2. In this case we would have to recommend using SHORT_TIMEOUT if users expect some test to be fast, am I right? Or should we just remove this from the docs completely?
  3. Should we change --timeout= flags that are used in the project somehow? https://cs.github.com/python/cpython?q=--timeout%3D



class BadStream(object):
Expand Down