bpo-30966: Add multiprocessing.SimpleQueue.close() #2760

vstinner · 2017-07-19T10:31:21Z

Add a new close() method to multiprocessing.SimpleQueue to explicitly
closes the queue. This is called automatically when the queue is
garbage collected.

pitrou · 2017-07-19T10:36:25Z

Lib/multiprocessing/queues.py

@@ -319,6 +319,13 @@ def __init__(self, *, ctx):
        else:
            self._wlock = ctx.Lock()

+    def __del__(self):


You shouldn't need that: the reader and writer already have a __del__.

Hum, maybe I gone too far. This change is part on a larger plan to add ResourceWarning everywhere.

I removed del(), sorry this change was not directly related to this PR. It can be added later if needed (for ResourceWarning).

mlouielu · 2017-07-19T10:36:41Z

Lib/test/_test_multiprocessing.py

+        queue = multiprocessing.SimpleQueue()
+        queue.close()
+        # closing a queue twice should not fail
+        queue.close()


Should this also need explicit testing _reader and _writer's .closed to be True?

I dislike testing private attributes in unit tests. Should I add a second @cpython_only unit test testing private attributes?

I added functional tests instead.

pitrou · 2017-07-19T10:36:55Z

Lib/test/_test_multiprocessing.py

+        queue = multiprocessing.SimpleQueue()
+        queue.close()
+        # closing a queue twice should not fail
+        queue.close()


What happens if I call get() or put() on a closed queue?

What happens if I call get() or put() on a closed queue?

No idea :-) Which behaviour do you expect/want?

Usually, it would raise a ValueError.

ok, let's do that.

serhiy-storchaka · 2017-07-19T11:18:27Z

Lib/multiprocessing/queues.py


    def get(self):
+        self._check_closed()


How much overhead adds this?

No idea. Does it matter? Correctness beats performance, no?

The performance matters. What bug fixed by adding this call?

@pitrou asked me to raise a ValueError.

There's so much going on here (I/O, pickling) that I don't think a trivial method call is going to matter.

There is a race condition here. AttributeError is raised if the queue is closed after calling _check_closed(). Maybe use locking in close() and guard the check with a lock?

SimpleQueue is supposed to be thread-safe? (I don't know the answer, it's a real question!) If we start to use the read lock, we also may have to use the write lock??

Otherwise why it use the locks?

serhiy-storchaka · 2017-07-19T11:21:01Z

Doc/whatsnew/3.7.rst

+---------------
+
+The :class:`multiprocessing.SimpleQueue` class has a new
+:meth:`~multiprocessing.SimpleQueue.close` method to explicitly closes the


I'm not sure, but seems there are errors in English here.

yup, same typo as below: should be "to explicitly close the..."

vstinner · 2017-07-19T11:21:53Z

I rewrote close() to close the writer even if closing the reader fails. It also clears _reader and _writer references to help the garbage collector.

I chose to remove the _poll attribute to simplify the implementation. But I don't know the rationale of the _poll() method. Was it added to add a private _poll() method? Or is it to prevent issues with the garbage collector on Python finalization?

SimpleQueue._poll attribute was added by the commit bdb1cf1 and http://bugs.python.org/issue12328:

    Issue #12328: Fix multiprocessing's use of overlapped I/O on Windows.
    Also, add a multiprocessing.connection.wait(rlist, timeout=None) function
    for polling multiple objects at once.  Patch by sbt.
    
    Complete changelist from sbt's patch:
    
    * Adds a wait(rlist, timeout=None) function for polling multiple
      objects at once.  On Unix this is just a wrapper for
      select(rlist, [], [], timeout=None).
    
    * Removes use of the SentinelReady exception and the sentinels argument
      to certain methods.  concurrent.futures.process has been changed to
      use wait() instead of SentinelReady.
    
    * Fixes bugs concerning PipeConnection.poll() and messages of zero
      length.
    
    * Fixes PipeListener.accept() to call ConnectNamedPipe() with
      overlapped=True.
    
    * Fixes Queue.empty() and SimpleQueue.empty() so that they are
      threadsafe on Windows.
    
    * Now PipeConnection.poll() and wait() will not modify the pipe except
      possibly by consuming a zero length message.  (Previously poll()
      could consume a partial message.)
    
    * All of multiprocesing's pipe related blocking functions/methods are
      now interruptible by SIGINT on Windows.

So @pitrou: Is it safe to remove _poll? Or should I keep it? If we keep it, I would prefer to explain why we need it.

cfbolz · 2017-07-19T12:57:32Z

Misc/NEWS.d/next/Library/2017-07-19-12-29-54.bpo-30966.QzFQPf.rst

@@ -0,0 +1,2 @@
+Add a new close() method to multiprocessing.SimpleQueue to explicitly closes


should be "to explicitly close the..."

oh right, fixed.

* Add a new close() method to multiprocessing.SimpleQueue to explicitly close the queue. * Operation on a closed queue raises a ValueError. * Remove the SimpleQueue._poll attribute: use self._reader.poll()

pitrou · 2017-07-20T13:12:49Z

Ok, I think the current diff has gone too far :-) If raising ValueError cannot be provided without adding a lot of locking and complication in the implementation, then I'll happily lose the ValueError requirement so that the implementation remains simple and lightweight (it's called "SimpleQueue" for a reason :-)).

vstinner · 2017-07-20T13:15:19Z

Ok, I think the current diff has gone too far :-) If raising ValueError cannot be provided without adding a lot of locking and complication in the implementation, then I'll happily lose the ValueError requirement so that the implementation remains simple and lightweight (it's called "SimpleQueue" for a reason :-)).

The complicated code comes from my will of setting attributes to None. Maybe if we only call .close() without setting attributes to None, it would simplify the code, no? What do you think?

pitrou · 2017-07-20T13:17:25Z

Maybe if we only call .close() without setting attributes to None, it would simplify the code, no? What do you think?

I don't know, I'd like to see the code :-)

vstinner · 2017-07-20T13:23:11Z

Ok, let's try something completely different: PR #2776 is a simpler queue with closer() ;-)

vstinner · 2017-08-10T23:41:48Z

Oh, I removed the branch by mistake :-p

vstinner requested a review from pitrou July 19, 2017 10:31

the-knights-who-say-ni added the CLA signed label Jul 19, 2017

pitrou reviewed Jul 19, 2017

View reviewed changes

mlouielu reviewed Jul 19, 2017

View reviewed changes

pitrou reviewed Jul 19, 2017

View reviewed changes

serhiy-storchaka reviewed Jul 19, 2017

View reviewed changes

cfbolz reviewed Jul 19, 2017

View reviewed changes

vstinner added 4 commits July 19, 2017 15:01

bpo-30966: Add multiprocessing.SimpleQueue.close()

95d15b4

* Add a new close() method to multiprocessing.SimpleQueue to explicitly close the queue. * Operation on a closed queue raises a ValueError. * Remove the SimpleQueue._poll attribute: use self._reader.poll()

Merge branch 'master' into simplequeue

4abadd2

Make SimpleQueue thread-safe.

1e0d693

Add unit test on SimpleQueue.__getstate__()

0c62914

vstinner mentioned this pull request Jul 20, 2017

bpo-30966: Add multiprocessing.SimpleQueue.close() #2776

Closed

vstinner closed this Aug 10, 2017

vstinner deleted the simplequeue branch August 10, 2017 23:39

vstinner mentioned this pull request Apr 27, 2020

bpo-30966: Add multiprocessing.SimpleQueue.close() #19735

Merged

		@@ -0,0 +1,2 @@
		Add a new close() method to multiprocessing.SimpleQueue to explicitly closes

Uh oh!

bpo-30966: Add multiprocessing.SimpleQueue.close() #2760

bpo-30966: Add multiprocessing.SimpleQueue.close() #2760

Uh oh!

Conversation

vstinner commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlouielu Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 20, 2017

Uh oh!

vstinner commented Jul 20, 2017

Uh oh!

pitrou commented Jul 20, 2017

Uh oh!

vstinner commented Jul 20, 2017

Uh oh!

vstinner commented Aug 10, 2017

Uh oh!

Uh oh!

mlouielu Jul 19, 2017 •

edited

Loading