Skip to content

bpo-37531: Fix regrtest timeout for subprocesses #14679

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 4 commits into from

Conversation

nanjekyejoannah
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah commented Jul 9, 2019

Fix regrtest timeout for subprocesses.

Naming things is the hardest thing in computer science. Interested in any reviews in a better name If any for the new function I introduced. cc @vstinner

https://bugs.python.org/issue37531

@ammaraskar
Copy link
Member

Hey @nanjekyejoannah, did you accidentally attach the wrong bpo ticket for this? 3753 seems really old and unrelated.

@ZackerySpytz
Copy link
Contributor

It seems that you have changed the file mode of several files under Lib/test. Why? I don't think those changes should be made.

@nanjekyejoannah nanjekyejoannah changed the title bpo-3753: Fix regrtest timeout for subprocesses bpo-37531: Fix regrtest timeout for subprocesses Jul 10, 2019
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.

It seems like you modified Lib/test/test_os.py permissions somehow, please revert this change.

@nanjekyejoannah
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

: please review the changes made to this pull request.

@nanjekyejoannah
Copy link
Contributor Author

Hey @nanjekyejoannah, did you accidentally attach the wrong bpo ticket for this? 3753 seems really old and unrelated.

Yes, thanks for catching this. I made the necessary changes.

@nanjekyejoannah
Copy link
Contributor Author

It seems that you have changed the file mode of several files under Lib/test. Why? I don't think those changes should be made.

oh yes, I have reverted this.

@vstinner
Copy link
Member

vstinner commented Aug 1, 2019

Overall, the change is great and seems to work as expected, but I tested it and it doesn't work as I expected:

  • it doesn't report stdout/stderr on timeout
  • regression: it ignores stdout/stderr on "CHILD_ERROR" whereas it was properly passed to the main process previously
  • faulthandler and Popen.communicate() use the same timeout, so it's unclear which one which trigger first: I tuned timeouts to give more time to faulthandler. I prefer faulthandler since it dumps the traceback where Python was stuck, whereas Popen.communicate() timeout only returns "truncated" output with no traceback
  • the timeout is reported as an exception, not as as "result"

I tried to add a commit to this PR but I wasn't allowed, so I forked this PR and created PR #15072 to add my changes on top of it.

@nanjekyejoannah
Copy link
Contributor Author

Closing this then in favor of PR #15072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants