| msg186687 - (view) |
Author: Sultan Qasim (sultanqasim) |
Date: 2013-04-13 01:30 |
| This issue seems to be new in Python 3.3.1. This worked fine in Python 3.3.0 and earlier. I am using fully up-to-date installation of Arch Linux, running the official arch repo's 64 bit build of Python 3.3.1. This issue is probably a result of the changes to multiprocessing's pipes brought about in solutions to issues #10527 or #16955. The multiprocessing Queue's get() method on Python 3.3.1 does not block on Linux when a timeout of 1 second or less is specified. I have not tested this on Windows or Mac OS X. Example Code: from multiprocessing import Queue q = Queue() q.get(True, 0.5) # Expected result: block for half a second before throwing exception # Actual result: throws empty exception immediately without waiting q.get(True, 1) # Expected result: block for one second before throwing exception # Actual result: throws empty exception immediately without waiting q.get(True, 1.00001) # Expected result: block for just over a second before throwing exception # Actual result: throws empty exception immediately without waiting q.get(True, 1.00002) # Blocks for just over a second, as expected q.get(True, 2) # Blocks for two seconds, as expected |
|
|
| msg186696 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2013-04-13 06:31 |
| Indeed, that's a regression introduced by fix for issue #10527. Just a rounding error: """ --- Lib/multiprocessing/connection.py.orig 2013-04-13 06:27:57.000000000 +0000 +++ Lib/multiprocessing/connection.py 2013-04-13 06:25:23.000000000 +0000 @@ -862,7 +862,7 @@ if hasattr(select, 'poll'): def _poll(fds, timeout): if timeout is not None: - timeout = int(timeout) * 1000 # timeout is in milliseconds + timeout = int(timeout * 1000) # timeout is in milliseconds fd_map = {} pollster = select.poll() for fd in fds: """ (the original patch really wasn't reviewed properly...) |
|
|
| msg186707 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2013-04-13 12:29 |
| Very good, regression #2 for 3.3.2. Keep them coming right now :) |
|
|
| msg186709 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-13 12:32 |
| My patch, my fault. I'm very sorry. |
|
|
| msg186710 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2013-04-13 12:33 |
| Don't worry, mistakes happen. My message was actually positive: it's better to catch the problems now than two weeks after the regression fix release... |
|
|
| msg186732 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-04-13 15:18 |
| As they say, s*** happens (one of my first patches caused a regression with thread-local storage in multiple interpreters setup, so...) Note that it's a strong case for selectors inclusion (issue #16853) :-) BTW, this would probably need a test, and since I'm abroad, I won't be able to commit it, so if someone could pick it up... |
|
|
| msg186831 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-13 20:31 |
| Assigning to me. Will get back in 1 or 2 days. |
|
|
| msg186951 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-14 21:19 |
| Patch including a unittest is in attachment. |
|
|
| msg186952 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-04-14 21:26 |
| Your test is much too strict (and I don't understand why you're using a Decimal). I think testing that the delta is greater or equal than 0.2 would be enough. |
|
|
| msg186953 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-14 21:29 |
| Yeah, right. Too strict indeed. I'll get rid of the assertLessEqual statement. Here's why Decimal is necessary: >>> import time >>> time.time() - time.time() -9.5367431640625e-07 >>> from decimal import Decimal >>> Decimal(time.time()) - Decimal(time.time()) Decimal('-0.000069141387939453125') |
|
|
| msg186954 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-04-14 21:32 |
| I don't understand what you're worrying about here. This is just because of evaluation order: >>> import itertools >>> it = itertools.count() >>> f = it.__next__ >>> f() - f() -1 >>> f() - f() -1 |
|
|
| msg186955 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-14 21:39 |
| Without using Decimal and without patching connections.py (hence q.get() returns immediately) the resulting delta is mismatched: ====================================================================== FAIL: test_timeout (__main__.WithProcessesTestQueue) ---------------------------------------------------------------------- Traceback (most recent call last): File "Lib/test/test_multiprocessing.py", line 708, in test_timeout self.assertGreaterEqual(delta, 0.19) AssertionError: 9.107589721679688e-05 not greater than or equal to 0.19 |
|
|
| msg186956 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-04-14 21:41 |
| > Without using Decimal and without patching connections.py (hence > q.get() returns immediately) the resulting delta is mismatched: Well, why are you surprised the test fails without the patch? |
|
|
| msg186958 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-14 21:56 |
| You're right, sorry. I got confused by the exponential notation in 9.107589721679688e-05. Updated patch is in attachment. |
|
|
| msg186968 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-04-15 05:57 |
| I think we should test multiple timeout values (e.g. 0.1, 0.5, 1 and 1.5): it'll take a little longer, but since the test suite didn't detect it, that's really lacking. Also, rathr than using an harcoded delta, we could maybe use a fudger factor, like what's done for threading lock tests. |
|
|
| msg186976 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-15 11:47 |
| Maybe I'm misinterpreting what you wrote but the test fails before the patch and succeeds after it so what's the point in adding multiple tests with different timeouts? > Also, rathr than using an harcoded delta, we could maybe use a fudger > factor, like what's done for threading lock tests. Not sure what you refer to here. Feel free to submit a patch if you want. |
|
|
| msg186997 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2013-04-15 15:00 |
| > Maybe I'm misinterpreting what you wrote but the test fails before the patch and succeeds after it so what's the point in adding multiple tests with different timeouts? Well, the test you added tests explicitely for a value < 1s because this specific bug was due to a rounding error, but I think it could be interesting to check a couple more values, within a reasonable range (not too long). It's just a matter of calling it in a loop, but if you don't deem it necessary, that's fine with me. > Not sure what you refer to here. Feel free to submit a patch if you want. See e.g. : http://hg.python.org/cpython/file/413c0b0a105f/Lib/test/lock_tests.py#l65 |
|
|
| msg187118 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2013-04-16 19:58 |
| I think I'll just stick with the original patch. |
|
|
| msg187154 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-04-17 11:10 |
| New changeset 65623d7dc76e by Giampaolo Rodola' in branch '3.3': Fix issue #17707: multiprocessing.Queue's get() method does not block for short timeouts. http://hg.python.org/cpython/rev/65623d7dc76e |
|
|
| msg187155 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2013-04-17 11:12 |
| New changeset 87882c96d138 by Giampaolo Rodola' in branch 'default': Fix issue #17707: multiprocessing.Queue's get() method does not block for short timeouts. http://hg.python.org/cpython/rev/87882c96d138 |
|
|