search

stripe / subprocess

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

Fix Errno::EMFILE with cwd caused by #29 #30

Closed andrew-stripe closed 7 years ago

andrew-stripe commented 7 years ago

Note to reviewers: The first commit here is a straight revert of #29. I'd suggest reviewing only the second commit (note that commit bumps the version two numbers because the revert rolled it back).

Using FileUtils.cd (an alias for Dir.chdir) triggers a warning message when Subprocess is called with the cwd arg from inside another chdir block: https://github.com/ruby/ruby/blob/v2_4_1/dir.c#L1033-L1034

In #29, I tried fixing this by using the block form of Dir.chdir but that caused many people to see errors of the form:

<elided>/subprocess-1.3.7/lib/subprocess.rb:280:in `exec': Too many open files - getcwd (Errno::EMFILE)

Instead of calling chdir in Ruby, I think it makes more sense to pass chdir as an argument to exec.

I still have to call Dir.chdir around a preexec_fn so that it continues to operate in the new working directory. We don't depend on that behavior anywhere at Stripe but it's possible someone else does. I suspect the bug was some interaction between Dir.chdir and exec so this feels safe.

cc @stripe/product-infra r? @nelhage

nelhage-stripe commented 7 years ago

Do you have any understanding of why this bug happened? It's extremely baffling to me why that might be the case.

:+1:

though if we're reasonably confident this resolves things.

andrew-stripe commented 7 years ago

After an IRL and some more digging in the Ruby codebase I still can't figure out where getcwd is being called in the exec flow nor why Dir.chdir would create problems with the number of open files. Going to go ahead and merge since I pretty clearly broke something... but I really wish I understood what's going on here.