Skip to content

bpo-30796: Fix failures in signal delivery stress test #2488

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

Merged
merged 2 commits into from
Jun 29, 2017

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jun 29, 2017

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

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
Copy link
Member Author

pitrou commented Jun 29, 2017

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

@vstinner
Copy link
Member

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
Copy link
Member Author

pitrou commented Jun 29, 2017

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
Copy link
Member Author

pitrou commented Jun 29, 2017

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
Copy link
Member Author

pitrou commented Jun 29, 2017

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

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

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?

Copy link
Member Author

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

Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

pitrou commented Jun 29, 2017

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

@pitrou pitrou merged commit f7d090c into python:master Jun 29, 2017
@pitrou pitrou deleted the fix_buildbot_setitimer branch June 29, 2017 14:40
@vstinner
Copy link
Member

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
* bpo-30796: Fix failures in signal delivery stress test

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

* Make sure to clear the itimer after the test
pitrou added a commit that referenced this pull request Jul 1, 2017
* [3.6] bpo-30703: Improve signal delivery (GH-2415)

* Improve signal delivery

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

* Remove unused function

* Improve comments

* Add stress test

* Adapt for --without-threads

* Add second stress test

* Add NEWS blurb

* Address comments @Haypo.
(cherry picked from commit c08177a)

* bpo-30796: Fix failures in signal delivery stress test (#2488)

* bpo-30796: Fix failures in signal delivery stress test

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

* Make sure to clear the itimer after the test
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.

3 participants