search

stripe / subprocess

A port of Python's subprocess module to Ruby
MIT License
208 stars 17 forks source link

Fix dup/close behavior #7

Closed zenazn closed 10 years ago

zenazn commented 10 years ago

This commit reverts 5ad3c3c9f787177b9313605ef05e1b6511224b69 and fixes the bug it was originally attempting to fix.

As of 5ad3c3c9f787177b9313605ef05e1b6511224b69, we dup IOs and file descriptors passed to Subprocess. This was to work around an issue where Subprocess would close files passed to it. However, the fact that Subprocess closed these files was the actual bug, and the dup'ing produced all kinds of undesirable behavior.

This change reverts the dup'ing behavior, but instead fixes the bug where Subprocess would close file descriptors you passed it, making the dup'ing behavior unnecessary in the first place.

zenazn commented 10 years ago

@ebroder, @ab — thoughts?

ebroder commented 10 years ago

This is semantically "only close the file descriptor if we created it", right? If so, LGTM

ab commented 10 years ago

:+1:

zenazn commented 10 years ago

Actually the last version was trying to be a bit too magic and didn't quite cover everything (although arguably the PIPE case is the most important). Could you guys have another look?

ab commented 10 years ago

LGTM!