Repository navigation
gh-87512: Fix ignored timeout in subprocess.run() on Windows - #155409
serhiy-storchaka wants to merge 2 commits into
Conversation
The pipes can be inherited by processes which outlive the child process, so reading them blocked until all of them exited, ignoring the timeout. The output is now read in chunks and the pending I/O is canceled, so the output read before the timeout is set on the TimeoutExpired exception, as on other platforms. Add _winapi.OpenThread(), _winapi.CancelSynchronousIo() and _winapi.THREAD_TERMINATE.
Documentation build overview
396 files changed ·
|
…rocess-timeout # Conflicts: # Modules/clinic/_winapi.c.h
gpshead
left a comment
gpshead
left a comment
There was a problem hiding this comment.
I'm not a Windows person so I'm mostly just going over this from a logical point of view and asking claude if these semantics make sense on Windows. I think this is workable but has flaws. So it still seems "better" (ie: landing it would be an improvement) but is not sufficient to fix the problem:
(claude's explanation below)
1. The cancel is one-shot and can miss. CancelSynchronousIo() only affects a thread that is inside ReadFile at that instant. If the reader is between PeekNamedPipe() and ReadFile(), or appending to the buffer, we get ERROR_NOT_FOUND (swallowed) and the thread then blocks in a read that nothing will cancel. In run() the following communicate() joins without a timeout, so we hang as before. The new tests can't hit this because the grandchild writes once and goes quiet; a grandchild that keeps writing would. I think this needs a "cancelled" flag that the reader loop checks before each read, with _cancel_io() repeating cancel + short join() until the thread exits.
2. A missed cancel in __exit__ leads to a read on a closed handle. The reader uses a raw handle captured once from get_osfhandle(). If the cancel misses, self.stdout.close() succeeds and the reader then calls ReadFile on a stale handle value: an unhandled OSError in the thread at best, a read from an unrelated reused handle at worst. The fix for 1 covers this too.
3. Could this use CancelIoEx(handle, NULL) instead? CancelSynchronousIo() is thread-targeted, so it cancels whatever synchronous I/O that thread happens to be doing (e.g. a finalizer running on it), and looking the thread up by thread.ident after is_alive() is open to thread ID reuse. My understanding is that CancelIoEx() also cancels synchronous I/O issued by other threads but is scoped to the pipe handle, which would avoid both problems and remove the need for OpenThread()/THREAD_TERMINATE. Is there a reason it doesn't work here? (It has the same miss race, so 1 still applies.)
4. Behaviour change: buffered data is dropped. fh.read() used to go through the BufferedReader/TextIOWrapper, so p.stdout.readline() followed by p.communicate() returned the rest. Reading the raw handle skips anything already buffered. POSIX already behaves this way, so this may be acceptable, but it should be a conscious decision and mentioned in the NEWS entry. [me: ugh!]
5. Is PeekNamedPipe() needed? ReadFile() on a byte-mode pipe returns as soon as any data is available, so ReadFile(handle, 32768) should give the same cancellable behaviour with half the syscalls, no peek-to-read window, and no 4K chunk cap on large outputs.
Smaller things:
- The stdin writer path is cancelled too but has no test (
input=with a full pipe). It relies on the aborted write surfacing asEINVALso_stdin_write()swallows it, and theself.stdin.close()after it may flush and block again. kill()thencommunicate()after acommunicate(timeout=...)on a barePopen(the pattern from the docs) still hangs, since onlyrun()and__exit__cancel. Fine if intentionally out of scope, but worth stating.OpenThreadlacks a docstring, and there are notest_winapitests for the new functions.
Points 3 and 5 are claude's recollection of CancelIoEx and pipe ReadFile semantics - that needs verification.
2 participants
The output is now read in chunks, using
PeekNamedPipe()to get the available size, so the reading threads are always blocked in a cancelable read. The pending I/O is canceled before closing the pipes and after the timeout inrun().As a result, the output read before the timeout is now set on the
TimeoutExpiredexception, as on other platforms.Added
_winapi.OpenThread(),_winapi.CancelSynchronousIo()and_winapi.THREAD_TERMINATE, as suggested by @eryksun.Note that
_communicate()now decodes the output at the end, as the POSIX implementation does, because the reading threads no longer read via theTextIOWrapper.