multiprocessing's overlapped PipeConnection on Windows

[sbt]

BPO	12328
Nosy	@pitrou, @tjguk, @briancurtin
Files	test_pipe_poll.py pipe_poll.patch pipe_poll_2.patch pipe_interruptible.patch sigint_event.patch pipe_poll_fix.patch PipeIO.zip pipe_poll_fix.patch pipe_poll_fix.patch pipe_poll_fix.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-03-05.18:33:14.011>
created_at = <Date 2011-06-13.18:53:16.048>
labels = ['extension-modules', 'type-bug']
title = "multiprocessing's overlapped PipeConnection on Windows"
updated_at = <Date 2014-03-20.08:28:43.153>
user = 'https://bugs.python.org/sbt'

bugs.python.org fields:

activity = <Date 2014-03-20.08:28:43.153>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2012-03-05.18:33:14.011>
closer = 'pitrou'
components = ['Extension Modules']
creation = <Date 2011-06-13.18:53:16.048>
creator = 'sbt'
dependencies = []
files = ['22345', '22350', '22366', '22367', '22488', '23736', '24340', '24388', '24730', '24737']
hgrepos = []
issue_num = 12328
keywords = ['patch']
message_count = 29.0
messages = ['138268', '138270', '138285', '138355', '138363', '138403', '138441', '138455', '138524', '139213', '139215', '148004', '148085', '148086', '148158', '148180', '148187', '148215', '148218', '152095', '152433', '154849', '154894', '154903', '154950', '154968', '154970', '155265', '214187']
nosy_count = 6.0
nosy_names = ['pitrou', 'tim.golden', 'jnoller', 'brian.curtin', 'python-dev', 'sbt']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12328'
versions = ['Python 3.3']

[sbt]

There are some problems with the new Windows overlapped implementation
of PipeConnection in the default branch.

1. poll(0) can return False when an empty string is in the pipe: if
   the next message in the pipe is b"" then PeekNamedPipe() returns
   (0, 0) which is indistiguishable from the case where the pipe is
   empty.

   This affects versions 2.6-3.2 of Python on Windows as well. For
   old versions I would just document the fact that poll() will fail
   to see that there is data in the pipe if the next message is an
   empty string. In practice this is not a big deal since pipes are
   primarily used for pickled objects which will never be the empty
   string.

2. _poll() forgets to check self._buffered, so it can return False
   even if there is buffered data.

3. Even if (2) is fixed, _poll() with a non-zero timeout can mess up
   message boundaries if the pipe contains messages of length zero or
   one. To fix this overlapped_GetOverlappedResult() needs to be
   changed to not swallow and forget ERROR_MORE_DATA errors.

4. In _poll() there is a race condition: if the read operation
   completes after WaitForMultipleObjects() timesout, but before the
   operation is cancelled, then the data read will be discarded.

5. The code assumes that if CancelIo()/CancelIoEx() returns
   successfully then the OVERLAPPED structure and associated buffer
   may be deallocated immediately. But the documentation says that if
   CancelIo()/CancelIoEx() succeeds then cancellation has only been
   *requested*:

   http://msdn.microsoft.com/en-us/library/aa363792%28v=vs.85%29.aspx

   If [CancelIoEx()] succeeds, the return value is nonzero. The
   cancel operation for all pending I/O operations issued by the
   calling thread for the specified file handle was successfully
   requested. The application must not free or reuse the
   OVERLAPPED structure associated with the canceled I/O operations
   until they have completed. The thread can use the
   GetOverlappedResult function to determine when the I/O operations
   themselves have been completed.

   We should wait for cancellation to *complete* by using
   GetOverlappedResult(..., TRUE), otherwise we risk a crash. If the
   operation was cancelled then FALSE is returned with GetLastError()
   == ERROR_OPERATION_ABORTED.

   I would suggest making it a programming error for the overlapped
   object to be deallocated while the operation is still pending, and
   to print a RuntimeError if that happens. (This does not prevent us
   from still trying to prevent a crash using CancelIoEx().)

6. Not a bug exactly, but poll(timeout) is no longer interruptible
   with Ctrl-C. This also means that Queue.get() is no longer
   interruptible.

--

The unit tests attached pass on Unix. On Windows versions up to 3.2,
only test_empty_string fails. On Windows version 3.3 all three fail.

[sbt]

The attached patch hopefully fixes problems (1)-(5), but I have never used overlapped I/O before. test_pipe_poll.py passes with these changes.

[pitrou]

Hello,

Thanks for the patch.

I would suggest making it a programming error for the overlapped
object to be deallocated while the operation is still pending, and
to print a RuntimeError if that happens. (This does not prevent us
from still trying to prevent a crash using CancelIoEx().)

Sounds ok to me.

6. Not a bug exactly, but poll(timeout) is no longer interruptible
   with Ctrl-C. This also means that Queue.get() is no longer
   interruptible.

There is infrastructure in _multiprocessing to handle Ctrl-C. Look for
"sigint_event" in Modules/_multiprocessing/*. This could be the topic of
a separate issue and patch.

The unit tests attached pass on Unix. On Windows versions up to 3.2,
only test_empty_string fails. On Windows version 3.3 all three fail.

Could you make the tests part of Lib/test/test_multiprocessing.py?

Also, what is the rationale for the following change:

•        elif timeout == 0.0:
  

+ elif timeout == 0.0 and nleft != 0:
return False

[sbt]

Also, what is the rationale for the following change:

•        elif timeout == 0.0:
  

•        elif timeout == 0.0 and nleft != 0:
             return False
  

If PeekNamedPipe() returns (navail, nleft) there are 3 cases:

1. navail > 0: just return True
2. navail = nleft = 0: either pipe is empty or next message is empty
   so fall through to slow path to find out which.
3. navail = 0, nleft > 0: a non-empty message is coming, but nothing
   is available yet.

The check is a shortcut for case 3, although it probably never
occurs. I've removed that check in the new patch, and added the
unit tests to test_multiprocessing.

There is infrastructure in _multiprocessing to handle Ctrl-C. Look
for "sigint_event" in Modules/_multiprocessing/*. This could be
the topic of a separate issue and patch.

I will look in to it.

[sbt]

pipe_interruptible.patch is a patch to support to making poll() interruptible. It applies on top of pipe_poll_2.patch.

I am not sure what the guarantees are for when KeyboardInterrupt will be raised.

I would have done it a bit differently if I knew a good way to test whether the current thread is the main one. Maybe there should be something like PyThread_is_main() and/or threading.ismain().

[pitrou]

pipe_interruptible.patch is a patch to support to making poll()
interruptible. It applies on top of pipe_poll_2.patch.

Hmm, it seems to me that it should be done in _poll() instead. Otherwise, recv() will not be interruptible, will it?

Also, after looking at this again, it seems sigint_event would be better provided (at the C level) by the Python core, e.g. as _PyOS_SigintEvent. The time module already uses a similar mechanism.

By setting the event in the sigint handler itself, rather than in an auxiliary HandlerRoutine callback function, it should also ensure that, when the event is set, the "tripped" bit in signalmodule.c has been set, meaning we wouldn't need to Sleep() anymore.

(I also wonder why the event isn't auto-reset. Otherwise, there's a race condition - which is probably harmless in most cases, but still.)

[sbt]

Hmm, it seems to me that it should be done in _poll() instead.
Otherwise, recv() will not be interruptible, will it?

Or maybe WaitForMultipleObjects() should be changed to also wait on sigint_event if called by the main thread.

Also, after looking at this again, it seems sigint_event would be
better provided (at the C level) by the Python core, e.g. as
_PyOS_SigintEvent. The time module already uses a similar mechanism.

By setting the event in the sigint handler itself, rather than in an
auxiliary HandlerRoutine callback function, it should also ensure
that, when the event is set, the "tripped" bit in signalmodule.c
has been set, meaning we wouldn't need to Sleep() anymore.

(I also wonder why the event isn't auto-reset. Otherwise, there's a
race condition - which is probably harmless in most cases, but still.)

If it is an auto-reset event then we must worry about a non-main thread stealing the event. Maybe _PyOS_SigintEvent() should return NULL if called by a non-main thread, and be documented with a loud usage warning.

[pitrou]

> Hmm, it seems to me that it should be done in _poll() instead.
> Otherwise, recv() will not be interruptible, will it?

Or maybe WaitForMultipleObjects() should be changed to also wait on
sigint_event if called by the main thread.

Indeed, this may be even better.

If it is an auto-reset event then we must worry about a non-main
thread stealing the event. Maybe _PyOS_SigintEvent() should return
NULL if called by a non-main thread, and be documented with a loud
usage warning.

You are right, we need a manual reset *or* we must ensure that every
user of _PyOS_SigintEvent only does so from the main thread.

[sbt]

You are right, we need a manual reset *or* we must ensure that every
user of _PyOS_SigintEvent only does so from the main thread.

On second thoughts, even using an auto-reset event, resetting the event before waiting is unavoidable. Otherwise you are liable to catch an event that was set a long time ago.

[sbt]

sigint_event.patch is a patch to make
_multiprocessing.win32.WaitForMultipleObjects interruptible. It
applies directly on to default.

The patch also adds functions _PyOS_SigintEvent and _PyOS_IsMainThread
which are implemented in signalmodule.c and declared in intrcheck.c.

_PyOS_SigintEvent returns a manual reset event (cast to void*) which
is set whenever SIGINT is received. It is Windows only.

_PyOS_IsMainThread returns 0 or 1 according to whether the current
thread is the main thread.

The time and _multiprocessing modules have been updated to use these
functions.

Note that WaitForMultipleObjects has a bWaitAll parameter. When this
is true, all handles in the array are waited for, and
WaitForMultipleObjects is not interruptible.

[sbt]

I have noticed a few more problems.

• Because poll() isn't thread safe on Windows, neither is Queue.empty(). Since a queue's pipe will never contain empty messages, this can be fixed easily by using (a wrapper for) win32.PeekNamedPipe().

• PipeListener/PipeClient have not been updated to use overlapped I/O.

• If more than one process is to read from a pipe connection then (even with proper synchronisation) we cannot safely use poll() since it can leave a partial message in the pipe.

[sbt]

Here is an updated patch (pipe_poll_fix.patch) which should be applied on top of sigint_event.patch.

It fixes the problems with PipeConnection.poll() and Queue.empty() and makes PipeListener.accept() use overlapped I/O. This should make all the pipe releated blocking functions/methods interruptible on Windows.

[python-dev]

New changeset 08953a04b2e6 by Antoine Pitrou in branch 'default':
Issue bpo-12328: Under Windows, refactor handling of Ctrl-C events and
http://hg.python.org/cpython/rev/08953a04b2e6