Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.gc() is async, do not attempt to invoke it and retry sysopen #2768

Merged
merged 2 commits into from Nov 2, 2016

Conversation

@eam
Copy link
Contributor

@eam eam commented Mar 27, 2015

IO objects abandoned while still open will call close() in their finalizer. Taking advantage of this behavior, MRI guards every fd allocating syscall retrying the syscall after running the GC if EMFILE/ENFILE are encountered:

https://github.com/ruby/ruby/blob/trunk/io.c#L5449-L5454
https://github.com/ruby/ruby/blob/trunk/dir.c#L485-L489
https://github.com/ruby/ruby/blob/trunk/ext/socket/socket.c#L243-L246

This is possible in MRI because rb_gc() blocks while the GC runs, guaranteeing that finalizers will have been invoked before the syscall is retried. System.gc() doesn't block, however, so this approach on the JVM will always fail.

The guards in jruby are also incomplete. As best I can tell they only exist around sysopen call, but don't exist around other descriptor allocating calls (directories, sockets, etc)

It seems to me that jruby could do one of several things:

  1. Remove the gc invocation and the sysopen retries (this PR). This should cause reliable failures when open IO objects accumulate.
  2. Remove the sysopen retries, but still invoke the gc. This should cause intermittent failures when open IO objects accumulate. My concern is this scenario may be more difficult to diagnose.
  3. Add this guard around the remaining descriptor allocating syscalls. Same concerns as option #2.

I don't know of a good way to emulate the behavior of MRI in this case, unfortunately.

@headius
Copy link
Member

@headius headius commented Jul 7, 2015

When I did the port of this and other similar code, I left this in mostly to keep the code consistent with MRI until I decided how to refactor it. I agree that this code is not effective in JRuby and it should be removed.

Your patch is mostly ok, but it leaves a nested if (fd == null) check in place. If you'd like to audit other places we call System.gc, they could probably benefit from similar treatment.

@eam
Copy link
Contributor Author

@eam eam commented Jul 8, 2015

Fixed the nested (fd == null) check, thanks.

Jruby doesn't seem to have the other EMFILE guards around other descriptor generating calls like Dir or Socket. I do see a few invocations in places such as AllocatedNativeMemoryIO. I'm not entirely clear on whether that invocation is effective, but given the context it probably doesn't hurt since it tests and throws.

@kares
Copy link
Member

@kares kares commented May 30, 2016

System.gc is still there (on 9.1.2.0) ... but the PR could use a rebase (or a hand merge)

@headius headius merged commit 466fee0 into jruby:master Nov 2, 2016
1 check failed
@headius headius added this to the JRuby 9.1.6.0 milestone Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants