NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.3k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

sobolevn wants to merge3 commits intopython:mainfromsobolevn:issue-46711

Closed

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

sobolevn wants to merge3 commits intopython:mainfromsobolevn:issue-46711

Conversation

Copy link

Member

sobolevn commentedFeb 11, 2022•
edited by bedevere-bot
Loading

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

However, I am not sure how to proceed here:

We can mergebpo-46679: Don't ignore timeout argument in test.support.wait_process. #31205 first, then I rebase this PR (this way the original author will get a credit for fixing the problem)
Or we can just merge this one (it is easier)

https://bugs.python.org/issue46711

bpo-46711: increase timeout for `test_logging::test_post_fork_child_n…

ae6e18a

…o_deadlock`

sobolevn requested a review fromvsajip as acode owner

February 11, 2022 09:48

the-knights-who-say-ni added the CLA signed label

Feb 11, 2022

bedevere-bot added awaiting review testsTests in the Lib/test dir labels

Feb 11, 2022

sobolevn requested a review fromvstinner

February 11, 2022 09:48

sobolevn added the skip news label

Feb 11, 2022

Usetimeout insupport.wait_process

c7c20a4

vstinner reviewed

Feb 11, 2022

View reviewed changes

Lib/test/test_logging.py

		@@ -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

vstinnerFeb 11, 2022

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

MemberAuthor

sobolevnFeb 11, 2022

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 nowSHORT_TIMEOUT is just 30 seconds, whileLONG_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 usingSHORT_TIMEOUT starts to fail randomly on slow buildbots, useLONG_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:

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

Copy link

Member

vstinner commentedFeb 11, 2022

Oh. I forgot that Idoubled 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 didhttps://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?

Copy link

Member

vstinner commentedFeb 11, 2022

cc@pablogsal

Copy link

Member

pablogsal commentedFeb 11, 2022

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

Copy link

Member

pablogsal commentedFeb 11, 2022

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 :(

Add info about timeout

b067c74

Copy link

MemberAuthor

sobolevn commentedFeb 11, 2022

So SHORT_TIMEOUT should now be 60 seconds rather than 30 seconds. Why didhttps://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.

Copy link

Contributor

notarealdeveloper commentedFeb 14, 2022•
edited by bedevere-bot
Loading

Closes#31205 Thank you,@notarealdeveloper for noticing it!
However, I am not sure how to proceed here:
We can merge [bpo-46679: Don't ignore timeout argument in test.support.wait_process. bpo-46679: Don't ignore timeout argument in test.support.wait_process. #31205](bpo-46679: Don't ignore timeout argument in test.support.wait_process. #31205) first, then I rebase this PR (this way the original author will get a credit for fixing the problem)
Or we can just merge this one (it is easier)
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.

vstinner reviewed

Feb 21, 2022

View reviewed changes

Copy link

Member

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?