classification
Title: setitimer() can disable timer by mistake
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: pitrou, vstinner
Priority: normal Keywords:

Created on 2017-06-29 19:49 by pitrou, last changed 2017-10-13 20:49 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2493 merged pitrou, 2017-06-29 19:54
PR 2497 merged pitrou, 2017-06-30 08:27
PR 2498 merged pitrou, 2017-06-30 08:28
PR 2499 merged pitrou, 2017-06-30 08:30
PR 3865 merged vstinner, 2017-10-03 12:24
Messages (26)
msg297300 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 19:49
The C setitimer() function supports intervals with microsecond resolution.  However, Python's setitimer() takes a float and then converts it to a C `struct timeval`, which can round down to zero.  The consequence is that the timer is disabled (timeval == {0,0}) while the user asked for a one microsecond timeout.
msg297306 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-29 20:13
I would prefer to reuse pytime.c conversion code which is well tested by test_time. _PyTime_FromObject() already contains code to convert a double to _PyTime_t, and _PyTime_AsTimeval() converts a _PyTime_t to timeval. Use _PyTime_ROUND_CEILING as signal.sigtimedwait().

For signal.sigtimedwait(): see commit 34dc0f46ae5c0c9ec91d9402fac61111b802855f and http://bugs.python.org/issue22117.

Using ROUND_CEILING is now also used in the select module for a similar reason.

See my https://haypo.github.io/pytime.html article for the horror story of timestamp rounding :-)
msg297308 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-29 20:15
I'm not sure about modifying Python 2.7. For example, we didn't change the select module in Python 2.7 to round correctly the timeout.
msg297309 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 20:17
It's not about correctly rounding something, it's about fixing a bug.  I don't care about rounding if it means being a microsecond late.
msg297310 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 20:17
> I would prefer to reuse pytime.c conversion code which is well tested by test_time. _PyTime_FromObject() already contains code to convert a double to _PyTime_t, and _PyTime_AsTimeval() converts a _PyTime_t to timeval. Use _PyTime_ROUND_CEILING as signal.sigtimedwait().

I don't understand how that works.  Can you post a snippet implementing what is needed here?
msg297311 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 20:19
sigtimedwait() is different: zero doesn't mean anything special.  If you mistake zero for 1e-6 or the reverse, it works fine.
msg297314 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-29 20:25
> sigtimedwait() is different: zero doesn't mean anything special.  If you mistake zero for 1e-6 or the reverse, it works fine.

Well, select.select() is a better example. I had nightmare with the
rounding of its timeout :-)
msg297316 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 20:27
> Well, select.select() is a better example. I had nightmare with the
rounding of its timeout 

Not sure why? If you pass 0 instead of a tiny value, select() shouldn't behave significantly differently.

In any case, I don't want to have nightmares, I simply want to fix a bug that contributed to make one the buildbots fail :-)
msg297318 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-29 20:44
> Not sure why? If you pass 0 instead of a tiny value, select() shouldn't behave significantly differently.

I used select() as an example of function where we also changed how the timeout was rounded, so we had to decide how to handle backward compatibility.

Rounding select() timeout has an big impact of power consumption in an event loop, especially when you use poll() which only has a resolution of 1 ms (and not 1 us). If you round 0.1 ms to 0, you enter a busy-loop which burns your CPU during 0.1 ms. If you do that many times, it can be much longer than 0.1 ms and so be very inefficient.

We had this performance issue in asyncio. We fixed it in asyncio *and* select modules. 

https://bugs.python.org/issue20311
msg297319 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 20:45
I don't understand how 0.1 ms can be rounded down to 0 in a "struct timeval", which has microsecond precision.
msg297320 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 20:46
Ok, it's not select(), it's epoll_wait().  I see.
msg297321 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 20:53
If I use the PyTime APIs, it will change the signature from
  setitimer($module, which, seconds, interval=0.0, /)
to
  setitimer($module, which, seconds, interval=None, /).

I'm not sure that's ok in a bugfix release?
msg297322 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-29 21:00
Also using the PyTime APIs will make it annoying to backport to 2.7 (which isn't mandatory, of course).
msg297344 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-30 07:45
Antoine Pitrou added the comment:
> If I use the PyTime APIs, it will change the signature from
>   setitimer($module, which, seconds, interval=0.0, /)
> to
>   setitimer($module, which, seconds, interval=None, /).
>
> I'm not sure that's ok in a bugfix release?

I understand that you want to use _PyTime_FromSecondsObject(), which
is the right function to use. This function uses PyFloat_AsDouble() or
PyLong_AsLongLong().

The current code uses:

    if (!_PyArg_ParseStack(args, nargs, "id|d:setitimer",
        &which, &seconds, &interval)) {
        goto exit;
    }

The "d" format uses PyFloat_AsDouble(). This function uses
Py_TYPE(op)->tp_as_number->nb_float(op) to convert an object to a
float.

Hum, it seems like the main difference is that
_PyTime_FromSecondsObject() doesn't handle objects defining
__float__() the same way. For example, _PyTime_FromSecondsObject()
rounds a decimal.Decimal to an integer, not a float :-/ It looks like
a bug in _PyTime_FromSecondsObject() which should first try to call
PyFloat_AsDouble(), catch TypeError and fallback to
PyLong_AsLongLong().
msg297346 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-30 07:46
> Also using the PyTime APIs will make it annoying to backport to 2.7 (which isn't mandatory, of course).

If you want to fix 2.7, I think that your simple test to round
"manually" to 1 microsecond is enough.
msg297348 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 07:50
It seems _PyTime_ObjectToTimeval() would be better here.  What do you think?
msg297350 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-30 07:58
Antoine Pitrou added the comment:
> It seems _PyTime_ObjectToTimeval() would be better here.  What do you think?

Oh, I forgot to document why _PyTime_ObjectToTimeval() still exists
:-) The _PyTime_t type supports a range of [-292 years, +292 years]:
it's enough to pass a timeout value to select.select() or
signal.setitimer(). But it's not enough to pass a timestamp relative
to the UNIX Epoch (1970-01-01 00:00), since we want to support crazy
dates like up to (0001-01-01 00:01) in the datetime module.

So the datetime module uses _PyTime_ObjectToTimeval() to use
internally the time_t type which supports a range of +/-
292,271,023,045 years when time_t is 64-bit long (which becomes the
case on most platforms) :-)

I would prefer to just fix _PyTime_FromSecondsObject() to handle
correctly decimal.Decimal, rather than using
_PyTime_ObjectToTimeval(). I tried to write an exhaustive test suite
for _PyTime_FromSecondsObject() which all corner cases, whereas
_PyTime_ObjectToTimeval() is less well tested and I would like to kill
it (but I don't know how at this point ;-)).

I can work on a patch, but not right now.

You can fix the bug differently if you prefer to not use pytime.
msg297353 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 08:00
Ok, I will just push the original fix then :-)
msg297354 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 08:01
New changeset 729780a810bbcb12b245a1b652302a601fc9f6fd by Antoine Pitrou in branch 'master':
bpo-30807: signal.setitimer() may disable the timer by mistake (#2493)
https://github.com/python/cpython/commit/729780a810bbcb12b245a1b652302a601fc9f6fd
msg297355 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 08:02
(I must admit your latest explanations lost me.  Why shouldn't I use _PyTime_ObjectToTimeval(), which is a convenience function, and instead use several functions in a row to get the right conversion?)
msg297356 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-30 08:02
What is the behaviour of setitimer() when the timeout is negative? Is this behaviour well defined (portable)?
msg297357 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 08:07
It "shall fail" apparently :-)
http://pubs.opengroup.org/onlinepubs/9699919799/functions/setitimer.html
msg297362 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 08:54
New changeset 6f3cb059fd1f3a57ffd70c9d54a394a85a6ea13d by Antoine Pitrou in branch '3.6':
[3.6] bpo-30807: signal.setitimer() may disable the timer by mistake (GH-2493) (#2497)
https://github.com/python/cpython/commit/6f3cb059fd1f3a57ffd70c9d54a394a85a6ea13d
msg297363 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 08:54
New changeset 5741b70acf88846a0d3b2d348535f250577b2df6 by Antoine Pitrou in branch '3.5':
[3.5] bpo-30807: signal.setitimer() may disable the timer by mistake (GH-2493) (#2498)
https://github.com/python/cpython/commit/5741b70acf88846a0d3b2d348535f250577b2df6
msg297364 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-06-30 08:54
New changeset a45a99b47ff241ce0ae2f0bba59b89e4e012d47c by Antoine Pitrou in branch '2.7':
[2.7] bpo-30807: signal.setitimer() may disable the timer by mistake (GH-2493) (#2499)
https://github.com/python/cpython/commit/a45a99b47ff241ce0ae2f0bba59b89e4e012d47c
msg304359 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-13 20:49
New changeset ef611c96eab0ab667ebb43fdf429b319f6d99890 by Victor Stinner in branch 'master':
bpo-30807: signal.setitimer() now uses _PyTime API (GH-3865)
https://github.com/python/cpython/commit/ef611c96eab0ab667ebb43fdf429b319f6d99890
History
Date User Action Args
2017-10-13 20:49:45vstinnersetmessages: + msg304359
2017-10-03 12:24:49vstinnersetpull_requests: + pull_request3845
2017-06-30 08:55:38pitrousetstatus: open -> closed
resolution: fixed
stage: backport needed -> resolved
2017-06-30 08:54:56pitrousetmessages: + msg297364
2017-06-30 08:54:34pitrousetmessages: + msg297363
2017-06-30 08:54:26pitrousetmessages: + msg297362
2017-06-30 08:30:38pitrousetpull_requests: + pull_request2560
2017-06-30 08:28:51pitrousetpull_requests: + pull_request2559
2017-06-30 08:27:29pitrousetpull_requests: + pull_request2558
2017-06-30 08:07:20pitrousetmessages: + msg297357
2017-06-30 08:02:58vstinnersetmessages: + msg297356
2017-06-30 08:02:14pitrousetstage: patch review -> backport needed
2017-06-30 08:02:07pitrousetmessages: + msg297355
2017-06-30 08:01:09pitrousetmessages: + msg297354
2017-06-30 08:00:37pitrousetmessages: + msg297353
2017-06-30 07:58:35vstinnersetmessages: + msg297350
2017-06-30 07:50:41pitrousetmessages: + msg297348
2017-06-30 07:46:36vstinnersetmessages: + msg297346
2017-06-30 07:45:13vstinnersetmessages: + msg297344
2017-06-29 21:00:48pitrousetmessages: + msg297322
2017-06-29 20:53:21pitrousetmessages: + msg297321
2017-06-29 20:46:16pitrousetmessages: + msg297320
2017-06-29 20:45:30pitrousetmessages: + msg297319
2017-06-29 20:44:03vstinnersetmessages: + msg297318
2017-06-29 20:35:36pitrousetstage: needs patch -> patch review
2017-06-29 20:27:29pitrousetmessages: + msg297316
2017-06-29 20:25:15vstinnersetmessages: + msg297314
2017-06-29 20:19:07pitrousetmessages: + msg297311
2017-06-29 20:17:59pitrousetmessages: + msg297310
2017-06-29 20:17:29pitrousetmessages: + msg297309
2017-06-29 20:15:15vstinnersetmessages: + msg297308
2017-06-29 20:13:14vstinnersetnosy: + vstinner
messages: + msg297306
2017-06-29 19:54:54pitrousetpull_requests: + pull_request2552
2017-06-29 19:49:24pitroulinkissue30796 dependencies
2017-06-29 19:49:10pitroucreate