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

Streams created for file parsing are allowed to finalize #5685

Closed
headius opened this issue Apr 10, 2019 · 2 comments

Comments

@headius
Copy link
Member

commented Apr 10, 2019

While looking into some tests that appear to be unreliable I discovered that we are adopting some JVM threads very early in execution due to early IO streams for parsing being allowed to close on their own via finalization.

Unreliable Test

The test I was investigating was CRuby's ruby/test_thread.rb in test_stop, which tests that stopping the sole thread (the main thread in a new subprocess) raises an error, since there are no other threads to schedule.

def test_stop
assert_in_out_err([], <<-INPUT, %w(2), [])
begin
Thread.stop
p 1
rescue ThreadError
p 2
end
INPUT
end

This test fails intermittently because we are adopting one of the JVM finalizer threads to clean up an IO object for the main script.

(It is not clear to me whether those threads should show up as Ruby threads for this purpose; typically these threads should remain hidden. I do not attempt to answer that question here.)

The orphaned IO

The IO that is left to finalize is the one created at this line:

io = RubyIO.newIO(runtime, Channels.newChannel(content));

The IO here is created for the parser, but nobody closes the stream after the parse is complete. This would be appropriate if it were held open for the magic DATA variable, but that's not the case here. The IO should be gracefully cleaned up when it's not needed anymore.

Because it is left to finalize, the finalizer thread is adopted into our thread lists, which breaks any tests that assume only threads created from Ruby code will be present. (Again, maybe we should be hiding adopted threads?)

I have confirmed that the other path here, for a main file opened as a FileInputStream, exhibits the same problem.

Fix

The best fix would be that the parser eagerly cleans up IO objects it uses for parsing once they are no longer needed. This will prevent any early finalization from needing to be adopted into our thread structures.

Workaround

If the streams are set to not autoclose, the finalizer will do nothing to them and will not need to be adopted. This is a low-impact fix, but it seems messy to leave them out there, and I believe this call path may be used for other load logic.

headius added a commit that referenced this issue Apr 10, 2019

@headius

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

cc @enebo

@enebo

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@headius ah yeah and it is two paths (other gets in underlying IO which is closed further up the call stack). I believe this is will be checking to see if DATA is that io and otherwise closing it in the finally of that same method. Should not be too hard. I will fix this.

@enebo enebo closed this in f81fe28 Apr 10, 2019

enebo added a commit that referenced this issue Apr 10, 2019

Fixes #5685. ruby io should be explicitly closed unless still used by…
… DATA (this time check constants and not globals)

enebo added a commit that referenced this issue Apr 10, 2019

@headius headius added this to the JRuby 9.2.8.0 milestone Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.