GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-46711: increase timeout for `test_logging::test_post_fork_child_no_deadlock` by sobolevn · Pull Request #31274 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Feb 11, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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. #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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sobolevn
bpo-46711: increase timeout for `test_logging::test_post_fork_child_n…
ae6e18a 
…o_deadlock`
@sobolevn
sobolevn requested a review from vsajip as a code owner February 11, 2022 09:48
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 11, 2022
@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Feb 11, 2022
@sobolevn
sobolevn requested a review from vstinner February 11, 2022 09:48
@sobolevn sobolevn added the skip news label Feb 11, 2022
@sobolevn
Use timeout in support.wait_process
c7c20a4
vstinner
vstinner reviewed Feb 11, 2022
View reviewed changes
Comment thread Lib/test/test_logging.py
lock_holder_thread.join()

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

@vstinner vstinner Feb 11, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sobolevn sobolevn Feb 11, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Feb 11, 2022

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Feb 11, 2022

Copy link
Copy Markdown
Member

cc @pablogsal
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal

pablogsal commented Feb 11, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal

pablogsal commented Feb 11, 2022

Copy link
Copy Markdown
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 :(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sobolevn
Add info about timeout
b067c74
@sobolevn

sobolevn commented Feb 11, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@notarealdeveloper

notarealdeveloper commented Feb 14, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.
sobolevn reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Feb 21, 2022
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner vstinner mentioned this pull request Apr 10, 2022
test_logging: test_post_fork_child_no_deadlock() failed with timeout on AMD64 Arch Linux Asan Debug 3.10 #90867
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@sobolevn sobolevn closed this Oct 8, 2022
@notarealdeveloper notarealdeveloper mannequin mentioned this pull request Apr 10, 2022
test.support.wait_process ignores timeout argument #90837
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@vsajip vsajip Awaiting requested review from vsajip vsajip is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@sobolevn @vstinner @pablogsal @notarealdeveloper @the-knights-who-say-ni @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.