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.

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
wants to merge 3 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 11, 2022

Closes #31205
Thank you, @notarealdeveloper for noticing it!

However, I am not sure how to proceed here:

https://bugs.python.org/issue46711

@@ -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)
Copy link
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?

Copy link
Member Author

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

@vstinner
Copy link
Member

Oh. I forgot that I doubled the ASAN buildbot last week: python/buildmaster-config@5a37411

    # Sometimes test_multiprocessing_fork times out after 15 minutes
    test_timeout = TEST_TIMEOUT * 2

Lib/test/libregrtest/setup.py adapts SHORT_TIMEOUT to --timeout option:

        # For a slow buildbot worker, increase SHORT_TIMEOUT and LONG_TIMEOUT
        support.SHORT_TIMEOUT = max(support.SHORT_TIMEOUT, ns.timeout / 40)

So SHORT_TIMEOUT should now be 60 seconds rather than 30 seconds. Why did https://buildbot.python.org/all/#/builders/621/builds/466 fail at 52.5 seconds seconds in this case?

Maybe my computation is wrong, or this build is older than the buildbot configuration change?

@vstinner
Copy link
Member

cc @pablogsal

@pablogsal
Copy link
Member

I'm ok not changing the default. I'm convinced by @sobolevn comments 👍

@pablogsal
Copy link
Member

So SHORT_TIMEOUT should now be 60 seconds rather than 30 seconds. Why did https://buildbot.python.org/all/#/builders/621/builds/466 fail at 52.5 seconds seconds in this case?

I have no idea. I checked and the builder should have picked the change you talk about but maybe I have missed something :(

@sobolevn
Copy link
Member Author

So SHORT_TIMEOUT should now be 60 seconds rather than 30 seconds. Why did https://buildbot.python.org/all/#/builders/621/builds/466 fail at 52.5 seconds seconds in this case?

I've added the information about the timeout to the error message. It can probably help the next time we have the same problem.

@notarealdeveloper
Copy link
Contributor

notarealdeveloper commented Feb 14, 2022

Closes #31205 Thank you, @notarealdeveloper for noticing it!

However, I am not sure how to proceed here:

https://bugs.python.org/issue46711

@sobolevn No worries! I don't mind not getting credit. :) This was just a minor thing I noticed while working on #30310.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

The Lib/test/support/init.py change is nice. I'm no longer sure that the Lib/test/test_logging.py change is correct. Maybe revert test_logging.py change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants