bpo-34044: subprocess.Popen copies startupinfo by vstinner · Pull Request #8090 · python/cpython (original) (raw)

segevfiner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember starting to implement this when adding lpAttributeList, since it's a list, before I discovered that subprocess clobbers STARTUPINFO anyhow.

@@ -135,6 +135,21 @@ def __init__(self, *, dwFlags=0, hStdInput=None, hStdOutput=None,
self.hStdError = hStdError
self.wShowWindow = wShowWindow
self.lpAttributeList = lpAttributeList or {"handle_list": []}
def copy(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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", []))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy.deepcopy() is only called 3 times in the whole stdlib... in dataclasses and mailbox.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😉

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,2 @@
subprocess.Popen now copy the startupinfo argument to leave it unchanged:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used your version would sounds better.

Nah, I dislike crediting myself. Git can be used for statistics ;-)