classification
Title: Multiprocessing queue get method does not block for short timeouts
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: benjamin.peterson, georg.brandl, giampaolo.rodola, larry, neologix, pitrou, python-dev, sbt, sultanqasim
Priority: release blocker Keywords: 3.2regression, 3.3regression, patch

Created on 2013-04-13 01:30 by sultanqasim, last changed 2013-04-17 11:18 by giampaolo.rodola. This issue is now closed.

Files
File name Uploaded Description Edit
issue17707.patch giampaolo.rodola, 2013-04-14 21:21 review
issue17707.patch giampaolo.rodola, 2013-04-14 21:56 review
Messages (20)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2013-04-13 12:32
My patch, my fault. I'm very sorry.
msg186710 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2013-04-13 20:31
Assigning to me. Will get back in 1 or 2 days.
msg186951 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-04-14 21:19
Patch including a unittest is in attachment.
msg186952 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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
History
Date User Action Args
2013-04-27 17:18:13neologixlinkissue17856 superseder
2013-04-17 11:18:37giampaolo.rodolasetkeywords: + 3.2regression, 3.3regression
status: open -> closed
resolution: fixed
versions: - Python 3.2
2013-04-17 11:12:43python-devsetmessages: + msg187155
2013-04-17 11:10:09python-devsetnosy: + python-dev
messages: + msg187154
2013-04-16 22:19:45giampaolo.rodolasetversions: - Python 2.7
2013-04-16 19:58:00giampaolo.rodolasetmessages: + msg187118
2013-04-15 15:00:05neologixsetmessages: + msg186997
2013-04-15 11:47:35giampaolo.rodolasetmessages: + msg186976
2013-04-15 05:57:17neologixsetmessages: + msg186968
2013-04-14 21:56:03giampaolo.rodolasetfiles: + issue17707.patch

messages: + msg186958
2013-04-14 21:41:07pitrousetmessages: + msg186956
2013-04-14 21:39:48giampaolo.rodolasetmessages: + msg186955
2013-04-14 21:32:37pitrousetmessages: + msg186954
2013-04-14 21:29:13giampaolo.rodolasetmessages: + msg186953
2013-04-14 21:26:56pitrousetnosy: + pitrou
messages: + msg186952
2013-04-14 21:21:47giampaolo.rodolasetfiles: + issue17707.patch
2013-04-14 21:21:39giampaolo.rodolasetfiles: - issue17707.patch
2013-04-14 21:19:32giampaolo.rodolasetfiles: + issue17707.patch
keywords: + patch
messages: + msg186951
2013-04-13 20:31:57giampaolo.rodolasetassignee: giampaolo.rodola
messages: + msg186831
2013-04-13 15:18:26neologixsetmessages: + msg186732
2013-04-13 12:33:09georg.brandlsetmessages: + msg186710
2013-04-13 12:32:01giampaolo.rodolasetmessages: + msg186709
2013-04-13 12:29:59georg.brandlsetmessages: + msg186707
2013-04-13 12:20:41pitrousetpriority: high -> release blocker
nosy: + larry, benjamin.peterson, georg.brandl, giampaolo.rodola
stage: patch review

versions: + Python 2.7, Python 3.2, Python 3.4
2013-04-13 06:31:59neologixsetpriority: normal -> high
2013-04-13 06:31:22neologixsetnosy: + neologix, sbt
messages: + msg186696
2013-04-13 01:30:35sultanqasimsettype: behavior
2013-04-13 01:30:25sultanqasimcreate