Skip to content
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

Improve stability of tests_subprocess #2190

Closed
wants to merge 1 commit into from
Closed

Improve stability of tests_subprocess #2190

wants to merge 1 commit into from

Conversation

tjstum
Copy link
Member

@tjstum tjstum commented Dec 6, 2021

On macOS, there appears to be some sort of race where sending SIGTERM
to a process "too early" in its lifetime causes the exit status to
appear as if SIGKILL was sent.
I can reproduce this with the stdlib subprocess. Additionally, this
problem doesn't occur if there is a tiny sleep between spawning a
child subprocess and terminating it.

My proposal is to use /bin/sleep on posix (which reliably does not
have this issue on macOS) and continue using the Python form elsewhere.

Related to #851

On macOS, there appears to be some sort of race where sending SIGTERM
to a process "too early" in its lifetime causes the exit status to
appear as if SIGKILL was sent.
I can reproduce this with the stdlib subprocess. Additionally, this
problem doesn't occur if there is a tiny sleep between spawning a
child subprocess and terminating it.

My proposal is to use `/bin/sleep` on posix (which reliably does not
have this issue on macOS) and continue using the Python form elsewhere.
@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #2190 (2e02839) into master (f8cb817) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2190   +/-   ##
=======================================
  Coverage   99.55%   99.55%           
=======================================
  Files         114      114           
  Lines       14751    14753    +2     
  Branches     2341     2343    +2     
=======================================
+ Hits        14685    14687    +2     
  Misses         44       44           
  Partials       22       22           
Impacted Files Coverage Δ
trio/tests/test_subprocess.py 100.00% <100.00%> (ø)

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

While this sounds good, we can't merge until we fix/silence the Fedora build which fails due to OpenSSL 3.0.

@tjstum
Copy link
Member Author

tjstum commented Dec 7, 2021

Should I delete those build plans, then?

@pquentin
Copy link
Member

pquentin commented Dec 7, 2021

Ideally we should use pytest.mark.xfail when detecting OpenSSL 3.0: https://docs.pytest.org/en/latest/how-to/skipping.html#condition-parameter

(Well ideally we should support OpenSSL 3.0, but that hasn't happened yet)

@tjstum tjstum closed this Dec 24, 2021
@tjstum tjstum deleted the fixtests branch December 24, 2021 00:16
@tjstum tjstum restored the fixtests branch December 24, 2021 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants