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

[vstinner]

Add a new close() method to multiprocessing.SimpleQueue to explicitly close the
queue.

[mention-bot]

@Haypo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @shibturn and @pitrou to be potential reviewers.

[vstinner]

My first attempt was the PR #2760 which tried to raise reliably a ValueError if the queue was closed. But according to @pitrou, the code became too complex because a queue must be thread-safe and I had to add too many code to make code thread-safe and prevent race condition with close().

This new attempt doesn't set _reader and _writer to None in close(), don't remove the _poll attribute, but don't raise check if the queue is closed. This task is delegated to the Connection class (_reader and _writer attributes).

[pitrou]

Ok, the fact that close() is not thread-safe worries me a bit (e.g. what happens if a thread calls close() while another thread is inside poll() or get()?).

Since this is meant to be used by multiprocessing.Pool, how about making it private (i.e. rename to _close() and remove the docs)?

[vstinner]

The issue asked to add a public close() method. For me, it makes sense to let the user call close() directly, as we do for files, sockets, etc.

Ok, the fact that close() is not thread-safe worries me a bit (e.g. what happens if a thread calls close() while another thread is inside poll() or get()?).

Sorry, I have no answer to this point.

[pitrou]

Actually, I think we are mistaken here. SimpleQueue is not meant to be thread-safe. It's meant to be usable from multiple processes (the locks are multiprocessing Lock instances). Each process gets its own SimpleQueue instance pointing to the same underlying resource (e.g. an anonymous pipe).

Of course, it can be a bit surprising that a multiprocess-safe object is not thread-safe, so I'm still a bit uncomfortable about this.

[vstinner]

Of course, it can be a bit surprising that a multiprocess-safe object is not thread-safe, so I'm still a bit uncomfortable about this.

In asyncio, I explicitly added notes to say that asyncio are not thread-safe by design. Would it be correct to add a similar note on SimpleQueue?

[pitrou]

Yes, that sounds good to me.

[pitrou]

Or, rather, that would really only concern the close() method.

[vstinner]

Or, rather, that would really only concern the close() method.

Sorry, but I'm a little bit lost. First, I understood that SimpleQueue is thread safe. Second, you say that in fact, locks are interprocess locks. Then I looked at Connection._send() and to me, it doesn't look thread safe:

    def _send(self, buf, write=_write):
        remaining = len(buf)
        while True:
            n = write(self._handle, buf)
            remaining -= n
            if remaining == 0:
                break
            buf = buf[n:]

_send() is not atomic: if write() is a partial write, thread A can send the begin of the buffer, and then thread B writes its data... I don't see how _send() could be atomic if the caller doesn't use a threading lock.

What are context locks? Interprocess locks? Threading locks? Both? :-)

[vstinner]

Oh, I removed the branch by mistake :-p