bpo-30796: Fix failures in signal delivery stress test by pitrou · Pull Request #2488 · python/cpython (original) (raw)

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

Already on GitHub?Sign in to your account

Conversation11 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

pitrou

setitimer() can have a poor minimum resolution on some machines, this would make the test reach its deadline (and a stray signal could then kill a subsequent test).

@pitrou

setitimer() can have a poor minimum resolution on some machines, this would make the test reach its deadline (and a stray signal could then kill a subsequent test).

@pitrou

@pitrou

@Haypo, do you like it? Another timer resolution for you :-)

@vstinner

Wow. IMHO calibrating setitmer is overkill for an unit test. The Python stdlib shouldn't depend too much on low-level OS features :-/

I suggest to remove the unit test. Maybe move it somewhere else?

@pitrou

IMHO calibrating setitmer is overkill for an unit test.

Well, it works and is short enough (less than 1 sec).

I suggest to remove the unit test. Maybe move it somewhere else?

Things that are moved elsewhere never get exercised, unfortunately.
The original PR fixes subtle race conditions that apparently are very difficult to diagnose and reproduce. The test case is a rare reliable way of reproducing them.

@pitrou

The Python stdlib shouldn't depend too much on low-level OS features :-/

This is test_signal.py. The entire point of its existence is to test our handling of low-level OS features :-)

@pitrou

I'd like to merge in order to see what the buildbots have to say.

vstinner

if reso <= 1e-4:
return 10000
elif reso <= 1e-2:
return 100

Choose a reason for hiding this comment

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

Isn't 100 enough on all platforms?

Choose a reason for hiding this comment

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

10000 triggers the issue much more reliably in my tests (it's very subtle).

Choose a reason for hiding this comment

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

"10000 triggers the issue much more reliably in my tests (it's very subtle)." ok

@vstinner

I'd like to merge in order to see what the buildbots have to say.

I'm not really opposed to the test. It's just that I don't want to maintain it :-) Since you seem to want to maintain it, I'm fine with it :-)

@pitrou

Thanks. If I really don't manage to make it stable enough, I'll restrict it to Linux :-)

@pitrou pitrou deleted the fix_buildbot_setitimer branch

June 29, 2017 14:40

@vstinner

Thanks. If I really don't manage to make it stable enough, I'll restrict it to Linux :-)

Ok, I agree with this plan. Let's see how buildbots like your code :-)

pitrou added a commit to pitrou/cpython that referenced this pull request

Jul 1, 2017

@pitrou

setitimer() can have a poor minimum resolution on some machines, this would make the test reach its deadline (and a stray signal could then kill a subsequent test).

pitrou added a commit that referenced this pull request

Jul 1, 2017

@pitrou

Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions.

setitimer() can have a poor minimum resolution on some machines, this would make the test reach its deadline (and a stray signal could then kill a subsequent test).