Close open file descriptors when calling Process.spawn #1904

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@lmars
lmars commented Sep 16, 2012

I based the change broadly on the changes in this commit from MRI.

However, I wasnt sure if changes similar to these (which extend the "update max fd" calls to pretty much everywhere open(2) or pipe(2) are called) where necessary (e.g. here)?

I attempted to do something similar to closefrom:

3.upto(255) do |fd|
  begin
    fd_to_io(fd).close_on_exec = true
  rescue Errno::EBADF
  end
end

but the process locked up, and I didnt investigate further.

@lmars
lmars commented Sep 20, 2012

This breaks the signal handler as it closes the file descriptors created here (and written to here and here) and the threads just hang waiting for eternity 😞

@brixen
Member
brixen commented Sep 22, 2012

Yeah, we can't just iterate a set of integers and randomly close file descriptors.

Perhaps a weakref set of all IO objects created. On Process.spawn with this option, we iterate the set, remove objects that have been collected in the parent, and close the fds in the child?

@lmars
lmars commented Sep 24, 2012

How about this?

@brixen brixen commented on an outdated diff Sep 25, 2012
kernel/common/io.rb
+ end
+
+ def add(*ios)
+ ios.each { |io| @ios << WeakRef.new(io) }
+ end
+
+ def each
+ @ios.each do |w|
+ obj = w.__object__
+ yield obj if obj and !obj.closed?
+ end
+ end
+ end
+
+ def self.open_ios
+ @open_ios ||= OpenIOGroup.new
@brixen
brixen Sep 25, 2012 Rubinius member

Just create this in the class body. Otherwise you need synchronization, which you don't want, nor do you want that test every time. You're never not going to want this object.

@brixen brixen and 1 other commented on an outdated diff Sep 25, 2012
kernel/common/io.rb
@@ -459,6 +459,32 @@ def self.setup(io, fd, mode=nil, sync=false)
io.sync = !!sync
io.sync ||= STDOUT.fileno == fd if STDOUT.respond_to?(:fileno)
io.sync ||= STDERR.fileno == fd if STDERR.respond_to?(:fileno)
+
+ open_ios.add(io)
+ end
+
+ # OpenIOGroup is used to maintain a WeafRef set of open IO
+ # objects, used in Process.spawn to close non standard file
+ # descriptors in child processes when necessary
+ class OpenIOGroup
+ def initialize
+ @ios = []
+ end
+
+ def add(*ios)
@brixen
brixen Sep 25, 2012 Rubinius member

This needs synchronization. Also, you should probably remove collected IOs at every N adds (perhaps N = 100).

@dbussink
dbussink Sep 25, 2012 Rubinius member

Any reason for using a splat here? I only see this being used with a single argument

edit:
Sorry, didn't completely read it, i see you also have cases with to IO objects

@lmars
lmars commented Sep 25, 2012

@brixen is this better?

@brixen
Member
brixen commented Sep 25, 2012

@lmars thanks for working on this. I think it's a reasonable solution. I wish we had some real world benchmarks we could check, but I suppose we will see if anyone reports slowness that shows a bottleneck here.

Just to reiterate, I think this is a superior solution to blacklisting special fds that we may use internally, primarily because the total number of fds can be set with ulimit, we'd need to check that value and iterate a possibly very big number. I don't see how doing that is a better solution.

@evanphx care to weigh in on this?

@dbussink
Member

One comment here, I actually preferred the original version of using an regular object instead of now having these class methods. Why not do something like this in the class body of IO:

@open_ios = OpenIOGroup.new
def self.open_ios
  @open_ios
end
@brixen
Member
brixen commented Sep 27, 2012

I prefer the instance approach as well. Creating the instance in the class body like @dbussink's code example was what my original comment suggested.

@lmars
lmars commented Sep 29, 2012

Is there a particular reason other than it feels better to have an instance rather than using class methods?

A downside of creating an instance is that it needs to be created in any subclass:

rbx -e 'Class.new(IO).pipe'
An exception occurred evaluating command line code
    undefined method `add' on nil:NilClass. (NoMethodError)

This is why initially I had:

def self.open_ios
  @open_ios ||= OpenIOGroup.new
end
@lmars
lmars commented Sep 29, 2012

Also, I'm struggling to figure out what is wrong with my initial implementation of calling close_on_exec on all file descriptors up to some maximum number?

All it should be doing is using fcntl(2) to set FD_CLOEXEC on underlying fds, but by doing so the signal handler pipe gets closed along the way?

Running the following:

# close_on_exec.rb
3.upto(1024) do |fd|
  begin
    IO.for_fd(fd).close_on_exec = true
  rescue Errno::EBADF
  end
end

causes:

$ rbx -X19 close_on_exec.rb 
SignalHandler::stop_thread failed to write: Bad file descriptor
@evanphx
Member
evanphx commented Sep 29, 2012

The problem is you're assuming that the semantics of all fds that Rubinius uses internally.

That being said, maybe you've uncovered a bug in the signal handler, you should look into that too.

@lmars
lmars commented Sep 29, 2012

@evanphx I don't think there is a bug in the signal handler, its just my brain failing on me.

I didn't realise there was an :autoclose option, so this works OK:

3.upto(1024) do |fd|
  begin
    IO.for_fd(fd, :autoclose => false).close_on_exec = true
  rescue Errno::EBADF
  end
end

Given this revelation, I am considering reverting back to tracking the max open fd and using that as the limit (i.e. like here).

WDYT?

@brixen
Member
brixen commented Oct 3, 2012

@lmars you make a good point about subclasses. However, that is also an issue with tracking the max_fd if a subclass overrides the open method. I'm wondering if we should track max_fd directly from the IO::open primitive.

After this discussion, I'm persuaded that tracking max_fd is probably the cleaner approach.

@lmars
lmars commented Oct 3, 2012

@brixen I've pushed up a modified version of my original solution: lmars@d9aa645

I've kind of got around the subclass issue I mentioned by calling update_max_open_fd explicitly on IO in the pipe methods, and putting the call in IO.setup which all subclasses should call.

@dbussink
Member

I manually merged this and moved the code for tracking the fd's to the VM. It needs to track it atomically and this way we make sure it's always tracked properly.

@dbussink dbussink closed this Oct 29, 2012
@lmars
lmars commented Oct 30, 2012

Great, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment