bpo-34044: subprocess.Popen copies startupinfo#8090
bpo-34044: subprocess.Popen copies startupinfo#8090vstinner merged 2 commits intopython:masterfrom vstinner:startupinfo
Conversation
|
@segevfiner: Would you mind to review my change? |
|
cc @eryksun |
segevfiner
left a comment
segevfiner
left a comment
There was a problem hiding this comment.
I remember starting to implement this when adding lpAttributeList, since it's a list, before I discovered that subprocess clobbers STARTUPINFO anyhow.
Lib/subprocess.py
Outdated
There was a problem hiding this comment.
This does a deep copy. Maybe it's better to implement this using copy.deepcopy? (That's how I thought about doing it when I originally discovered that subprocess modifies the STARTUPINFO)
That way there will be no need to manually deep copy specific attributes by name below. You can just copy.deepcopy(self.lpAttributeList, memo). This should boil this entire method into one statement.
There was a problem hiding this comment.
IMHO copy.deepcopy() is a bad practice. It seems like lpAttributeList can only contain one key, "handle_list", and the value of this key is always converted to a list in Popen (simplified code):
handle_list = list(attribute_list.get("handle_list", []))
There was a problem hiding this comment.
copy.deepcopy() is only called 3 times in the whole stdlib... in dataclasses and mailbox.
There was a problem hiding this comment.
I also came up with this alternative:
attribute_list = self.lpAttributeList.copy()
if 'handle_list' in attribute_list:
attribute_list['handle_list'] = list(attribute_list['handle_list'])It's shorter, and dict.copy might be faster than iterating and assigning to a new dict (I didn't check...) Use whichever one you like best. 😉
There was a problem hiding this comment.
Maybe this sounds better?
``subprocess.Popen`` now copies the ``startupinfo`` argument to leave it unchanged:
it will modify the copy, so that the same ``STARTUPINFO`` object can be used multiple times.
Not sure if core members add a Patch by note...
There was a problem hiding this comment.
I used your version would sounds better.
Nah, I dislike crediting myself. Git can be used for statistics ;-)
subprocess.Popen now copies the startupinfo argument to leave it unchanged: it will modify the copy, so that the same STARTUPINFO object can be used multiple times. Add subprocess.STARTUPINFO.copy() method.
|
I used git push --force to rewrite the commit message. |
|
@segevfiner: Does the new version look better? |
Thank you for the review! |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
GH-8120 is a backport of this pull request to the 3.7 branch. |
subprocess.Popen now copies the startupinfo argument to leave it unchanged: it will modify the copy, so that the same STARTUPINFO object can be used multiple times. Add subprocess.STARTUPINFO.copy() method. (cherry picked from commit 483422f) Co-authored-by: Victor Stinner <[email protected]>
|
I don't think that it's ok to add a public STARTUPINFO.copy() method in 3.7.1, so I created a backport manually to make the method private: PR #8121. |
subprocess.Popen now copies the startupinfo argument to leave it unchanged: it will modify the copy, so that the same STARTUPINFO object can be used multiple times. Add subprocess.STARTUPINFO._copy() private method. Python 3.7 backport from master makes the copy() private: renamed to _copy(). (cherry picked from commit 483422f)
5 participants
subprocess.Popen now copy the startupinfo argument to leave it
unchanged: modify the copy, so the same startupinfo object can be
used multiple times.
https://bugs.python.org/issue34044