| msg297300 - (view) |
Author: Antoine Pitrou (pitrou) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2017-06-29 20:46 |
| Ok, it's not select(), it's epoll_wait(). I see. |
|
|
| msg297321 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
Date: 2017-06-30 07:50 |
| It seems _PyTime_ObjectToTimeval() would be better here. What do you think? |
|
|
| msg297350 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
Date: 2017-06-30 08:00 |
| Ok, I will just push the original fix then :-) |
|
|
| msg297354 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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 |
|
|