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

JVM finalizer threads get adopted into JRuby thread list #4960

Open
headius opened this issue Jan 9, 2018 · 2 comments
Open

JVM finalizer threads get adopted into JRuby thread list #4960

headius opened this issue Jan 9, 2018 · 2 comments

Comments

@headius
Copy link
Member

headius commented Jan 9, 2018

This may or may not be a real problem, but it causes many tests to fail sporadically, such as this one from MRI's test_thread.rb which tests that Thread.list has the correct number of threads in it:

  def test_list
    assert_in_out_err([], <<-INPUT) do |r, e|
      t1 = Thread.new { sleep }
      Thread.pass
      t2 = Thread.new { loop { Thread.pass } }
      Thread.new { }.join
      p [Thread.current, t1, t2].map{|t| t.object_id }.sort
      p Thread.list.map{|t| t.object_id }.sort
    INPUT
      assert_equal(r.first, r.last)
      assert_equal([], e)
    end
  end

There are also tests in RubySpec that occasionally see extra threads in lists.

After some exploration today I figured out that in the case of the MRI test above (and likely in many of the other cases) the extra thread(s) are from the JVM's finalization subsystem. The JVM runs object finalization in a VM-started thread that enumerates all queued up soon-to-be-dead references and runs the finalizers associated with those objects. If those finalizers need to run Ruby code, the finalizer thread will be "adopted" into JRuby, showing up in Thread.list among other places.

This causes the tests to fail sporadically, since sometimes finalization has started and sometimes it has not; sometimes finalization needs to interact with Ruby and sometimes not.

I'm not sure if there's a fix for this. It is not unreasonable for us to have extra VM threads; Thread.list expectations imply that user code is the only thing allowed to start threads. And any thread that runs Ruby code does need to be adopted.

My thought, at the moment, is that perhaps including all adopted threads in Thread.list is not appropriate. With some tweaking, those threads could be marked as such so they would not be included. This would potentially break embedding users that expect non-JRuby threads to be in the list, but providing an alternative mechanism may be enough.

Alternatively, if we can determine a thread is from the JVM finalization subsystem, we could exclude just that one thread. But this feels too cookie-cutter to me.

There's also the looming requirement that we move away from finalization. Object.finalize is deprecated as of Java 9, with the preferred alternative likely being phantom references and a userland finalization thread. Again, we might have a VM-level thread showing up in the Ruby thread list.

headius added a commit that referenced this issue Jan 9, 2018

Verified

This commit was signed with the committer’s verified signature.
headius Charles Oliver Nutter
JRuby's list of threads may include those that come from outside
JRuby itseld, like JVM finalizer or worker threads. Because this
might happen at some unknown time in the future, it causes tests
relying on Thread.list to fail sporadically.
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch jruby-9.1
# Your branch is up-to-date with 'origin/jruby-9.1'.
#
# Changes to be committed:
#	modified:   test/mri/excludes/TestThread.rb
#
# Untracked files:
#	bench1.rb
#	bench2.rb
#	blah
#	blah.rb
#	blah.sh
#	blah2.rb
#	jruby_jetpdb/
#	link20180108-44247-163yxpr
#	nic.rb
#	ripper_bug_4792.rb
#	run_jruby.sh
#	test/jruby/tcttac37356-0.jpg
#	test/jruby/testapp/testapp
#	test_thr_kill_count
#	testapp2/
#	tmp/
#	var/
#	"\316\261.rb"
#	"\343\201\202"
#
@enebo
Copy link
Member

enebo commented Jan 9, 2018

Ah this is a cool find. There is a duality which is difficult to resolve in adoption. If you are an embedder you may think it should be in this list because you made it. OTOH for a case like finalization I suspect no one expects to see it except by experience of having noticed it in passing (like you did).

So in the first case it should be in the list as the user thinks it is a Ruby thread. In the later case I do not think people would expect that to show up; Although perhaps there should be a way of seeing that from Ruby space? Special casing adoption due to finalization as an implicit impl-specific backend thing and not displaying does not feel cookie cutter to me. At least not from the explanation I gave above. It is possible we maybe even make more background impl-specific threads which may end up doing callMethod but I really don't see those as showing up in Thread.list either?

@headius
Copy link
Member Author

headius commented Jan 9, 2018

Well there's a few places where we'll see threads that you wouldn't normally see in MRI:

  • Finalizer threads
  • Worker threads spun up by our own executors or by executors from elsewhere on the JVM
  • Fibers and enumerators I believe show up when they're running in a thread
  • Embedding call-ins

The problem I see is that it's hard to manage the thread context that goes with a given adopted thread unless we put it in the same structures we build Thread.list from. Of course, there's no reason we can't flag external threads as "adopted" and not include them when we build the list. Bottom line: we can't get rid of the adoption plumbing, but we may be able to hide it.

Another option to reduce the risk of losing threads people expect to see: we could have this behavior on only when running from command line (i.e. through org.jruby.Main). Rubyists would be far less likely to complain.

Still another possibility: threads spun up by the JVM or by third-party code will have their own thread group. We could apply a runtime-specific thread group to all JRuby threads, and then we'd always know which are ours and which aren't. We can present that in Thread.list however is appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants