Code review comment for lp:~broder/ubuntu-dev-tools/fix-785854

Revision history for this message
Evan Broder (broder) wrote :

I don't know of any problems with making ubuntutools.subprocess.Popen a subclass of subprocess.Popen, so I've done so.

As for the design consideration, I really do think there's value in making it easy to drop in this replacement that has better defaults, since the fd- and signal-related bugs around subprocess are easy to forget about.

Certainly I don't know of any cases where you *wouldn't* want to reset the signal handlers (since anything that expects them to be set specially - like, say, a Python process - will just set them again on startup).

I think close_fds isn't quite as totally uncontroversial, but I've also seen way more weird problems caused by passing fds to children (e.g. the other end of process 1's stdin ends up in process 2's fd space, even though parent process has its copy of that fd, leading to process 1 hanging until process 2 exits). When you deliberately mean to pass fds through, I think it's fairly obvious what's going wrong.

So I think that it's good to make it incredibly easy to use ubuntutools.subprocess over subprocess itself, at least within u-d-t.

« Back to merge proposal