[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Sebastian Bank added the comment: Thanks Eryk. Done: https://bugs.python.org/issue34044 -- ___ Python tracker ___ ___

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Eryk Sun added the comment: Sebastian, the problem in this case is that startupinfo.lpAttributeList['handle_list'] contains the duplicated standard-handle values from the previous call, which were closed and are no longer valid. subprocess.Popen has always modified STARTUPINFO in place,

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Sebastian Bank added the comment: AFAIU, this change broke the following usage of subprocess on Windows (re-using a subprocess.STARTUPINFO instance to hide the command window): import os, subprocess STARTUPINFO = subprocess.STARTUPINFO() STARTUPINFO.dwFlags |=

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: Thank you Sergev Finer for finishing the implementation of my PEP 446. Supporting to only inherit a set of Windows handles was a "small note" my PEP 446, mostly because I didn't feel able to implement the feature, but also because

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: Copy of my comment on the PR. https://github.com/python/cpython/pull/1218#issuecomment-352372211 > Merged from master... Again... Hopefully this won't end up missing 3.7 > entirely...  Oops sorry, I wanted this feature but I

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: New changeset b2a6083eb0384f38839d3f1ed32262a3852026fa by Victor Stinner (Segev Finer) in branch 'master': bpo-19764: Implemented support for subprocess.Popen(close_fds=True) on Windows (#1218)

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: The PR has been sitting there for quite a while now... -- ___ Python tracker ___

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Eryk Sun added the comment: We can't reliably distinguish file descriptors from OS handles. They're overlapping sets of integers. A separate pass_handles parameter would be needed. The bigger problem with that idea is that the handles in the list have to be made inheritable before calling

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Gregory P. Smith added the comment: I am not a Windows person... but is there a reason that handle_list must be an attribute of a STARTUPINFO class rather than just a special case of pass_fds such that people could supply windows handles in the pass_fds parameter rather than using

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: GitHub PR bit rotting away... :P Just a friendly reminder :) -- ___ Python tracker ___

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: OK Rietveld definitely punted on the git patch (I guess it's only for the old Mercurial repo, I don't think it actually even support Git...) I will try re-submitting the patch as a PR so that it can be reviewed easily. --

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Changes by Roundup Robot : -- pull_requests: +1341 ___ Python tracker ___

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: Oh LOL!!! I missed the fact that Python finally moved to GitHub! Rebased the patch on top of the Git master XD (And removed accidentally committed code... sorry...) I still submitted as a patch since I don't know if the infrastructure handles moving a patch to a

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: Added the 5th version after another review by eryksun (In rietveld). -- Added file: http://bugs.python.org/file46819/windows-subprocess-close-fds-v5.patch ___ Python tracker

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: Added the 4th version after review by eryksun (In rietveld). -- Added file: http://bugs.python.org/file46814/windows-subprocess-close-fds-v4.patch ___ Python tracker

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Eryk Sun added the comment: In case you didn't get notified by Rietveld, I made a couple suggestions on your latest patch. Also, if you wouldn't mind, please update the patch to apply cleanly to 3.7 -- especially since STARTUPINFO now has an __init__ method. --

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: It's been a while since this got any attention... -- ___ Python tracker ___

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: I have read some of https://github.com/rprichard/win32-console-docs and it documents quite a bunch of nastiness with PROC_THREAD_ATTRIBUTE_HANDLE_LIST in Windows Vista/7. Windows is so much fun sometimes :P Essentially console handles in Windows before Windows 8

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Eryk Sun added the comment: > Python already has a multiprocessing module which is able to pass > handles (maybe also FD? I don't know) to child processes on > Windows. Popen doesn't implement the undocumented CRT protocol that's used to smuggle the file-descriptor mapping in the STARTUPINFO

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: I removed previous_handle_list in _execute_child since I noticed subprocess already clobbers the other attributes in startupinfo anyhow. I figured there will be some discussion about how to pass the handle list, so here's my two cents: * subprocess already

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: I dislike adding a lpAttributeList attribute: it's too close to the exact implementation of Windows may change in the future. I would prefer a more high level API. Since the only known use case today is to pass handles, I propose to focus on this use case:

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: Python already has a multiprocessing module which is able to pass handles (maybe also FD? I don't know) to child processes on Windows. I found some code in Lib/multiprocessing/reduction.py: - duplicate() - steal_handle() - send_handle() But the design doesn't

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: Second version of the patch after review by eryksun. Please pay attention to the hack in _execute_child due to having to temporarily override the handle_list if the user supplied one. As for pass_fds: as you noted, it has it's own share of complexities and

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Eryk Sun added the comment: Implementing pass_fds on Windows is a problem if Popen has to implement the undocumented use of the STARTUPINFO cbReserved2 and lpReserved2 fields to inherit CRT file descriptors. I suppose we could implement this ourselves in _winapi since it's unlikely that the

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Segev Finer added the comment: Though Python has taken measures to mark handles as non-inheritable there is still a possible race due to having to create inheritable handles while creating processes with stdio pipes (subprocess). Attached is a Patch that implements

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

Changes by STINNER Victor : -- components: +Windows nosy: +paul.moore, steve.dower, tim.golden, zach.ware type: enhancement -> resource usage ___ Python tracker

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: I read again the following blog post: http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx I understood the purpose of PROC_THREAD_ATTRIBUTE_HANDLE_LIST. Let say that two Python threads create a Popen object with a pipe for stdout: * Thread A

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: The purpose of this issue is to avoiding having to call CreateProcess() with bInheritHandles parameter set to TRUE on Windows, and avoid calls to self._make_inheritable() in subprocess.Popen._get_handles(). Currently, bInheritHandles is set to TRUE if stdin,

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

STINNER Victor added the comment: UpdateProcThreadAttribute() documentation says that ... handles must be created as inheritable handles ... and a comment says that If using PROC_THREAD_ATTRIBUTE_HANDLE_LIST, pass TRUE to bInherit in CreateProcess. Otherwise, you will get an

[issue19764] subprocess: use PROC_THREAD_ATTRIBUTE_HANDLE_LIST with STARTUPINFOEX on Windows Vista

New submission from STINNER Victor: subprocess.Popen has a race condition on Windows with file descriptors: if two threads spawn subprocesses at the same time, unwanted file descriptors may be inherited, which lead to annoying issues like cannot delete a file because it is open by another