-
-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-42178: Fix issue causing cmd to hang #23290
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Try writing subprocess data before creating subprocess
https://bugs.python.org/issue42178