bpo-42178: Fix issue causing cmd to hang #23290
Conversation
Try writing subprocess data before creating subprocess
| reduction.dump(process_obj, f) | ||
| finally: | ||
| set_spawning_popen(None) | ||
|
|
There was a problem hiding this comment.
Pickling Semaphore or SemLock objects entails duplicating an OS handle to the child process via duplicate_for_child. So we need the child process in order to do the reduction only once. What we can do to improve the situation is to spawn the child process with a suspended thread. This will require adding the CREATE_SUSPENDED flag and ResumeThread function to the _winapi module. After creating the child process with a suspended thread, set up the self attributes (e.g. self.sentinel) and dump the reduction to a BytesIO instance. If the latter fails, kill the child process via _winapi.TerminateProcess. Otherwise resume the thread in the child, and write the reduction from the BytesIO instance to the pipe.
|
Hi. I implemented the review feedback. |
Modules/_winapi.c
Outdated
| @@ -1553,6 +1553,28 @@ _winapi_ReadFile_impl(PyObject *module, HANDLE handle, DWORD size, | |||
| return Py_BuildValue("NI", buf, err); | |||
| } | |||
|
|
|||
| /*[clinic input] | |||
| _winapi.ResumeThread | |||
There was a problem hiding this comment.
I don't see the point in throwing away the thread's previous suspend count when it's so easy to just return it. I think you should change this line of the definition to _winapi.ResumeThread -> DWORD, and return result. It could be useful later on for some other problem.
| reduction.dump(prep_data, f) | ||
| reduction.dump(process_obj, f) | ||
| except: | ||
| _winapi.TerminateProcess(self._handle) |
There was a problem hiding this comment.
You need the exit code: _winapi.TerminateProcess(self._handle, 1). Also add a raise statement to propagate the exception to the caller.
| _winapi.TerminateProcess(self._handle) | ||
| else: | ||
| f.seek(0) | ||
| to_child.write(f.read()) |
There was a problem hiding this comment.
The thread needs to be resumed before writing to the pipe. Writing to the pipe will block if it fills up, and by default CreatePipe uses a 4 KiB buffer, i.e. a write that exceeds 4096 bytes will block. That's not an argument to specify a larger buffer size when creating the pipe. We just need the child to be running, so it can steal rhandle and read from its end.
Also, there's no need to seek the BytesIO instance. Just use f.getvalue() instead of f.read().
| f.seek(0) | ||
| to_child.write(f.read()) | ||
| _winapi.ResumeThread(ht) | ||
| _winapi.CloseHandle(ht) |
There was a problem hiding this comment.
Move closing the thread handle to the first line of the finally block.
Modules/_winapi.c
Outdated
| if (result == -1) | ||
| return PyErr_SetFromWindowsErr(GetLastError()); | ||
|
|
||
| return PyLong_FromLong((int) result); |
There was a problem hiding this comment.
There's already a converter for the DWORD type, which is used for GetExitCodeProcess, GetLastError, and GetFileType. Just change the clinic input to return a DWORD.
|
Hello. I made the requested changes. |
|
This PR is stale because it has been open for 30 days with no activity. |
github-actions
bot
removed
the
stale
Try writing subprocess data before creating subprocess
https://bugs.python.org/issue42178