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

Java class proxy creation deadlock #1621

Closed
cky opened this issue Apr 11, 2014 · 7 comments
Closed

Java class proxy creation deadlock #1621

cky opened this issue Apr 11, 2014 · 7 comments

Comments

@cky
Copy link
Contributor

@cky cky commented Apr 11, 2014

Affects version 1.7.11, and probably a whole bunch of versions prior.

Synopsis

Java class proxy creation happens inside of a mutex lock, one per proxy. If the class has other dependencies, such as constant fields, superclasses, etc., the proxy for the dependent type is created while still inside the lock.

If the classes have circular dependencies (this especially comes up for enum values with class bodies, which are implemented as subclasses of the enum type), let's say classes A and B, each of which has a constant field of the other type, then two threads could simultaneously request a proxy for each of those classes. This results in deadlock.

Proof-of-Concept

I've attached a proof-of-concept of this (tested on JRuby 1.7.11). Here's deadlock_test.rb:

#!/usr/bin/env jruby
(:A..:H).map {|sym| Thread.new {Java::Default.const_get sym}}.each(&:join)

And here's A.java (B through H are similar; H::NEXT links back to A::INSTANCE, so that we have an 8-class daisy-chain dependency cycle):

public class A {
    static final A INSTANCE = new A();
    public static final B NEXT = B.INSTANCE;
}

(Of course, you can replicate this with only 2 classes, but using 8 significantly widens the deadlock window; in 10 test runs on my computer, I've achieved deadlock for all 10 runs using 8 classes, whereas only 4 of 10 runs resulted in deadlock when using 2 classes.)

@cky
Copy link
Contributor Author

@cky cky commented Apr 15, 2014

Just tested it with the newly-released 1.7.12. Yep, problem still persists. :-)

@jmiettinen
Copy link
Contributor

@jmiettinen jmiettinen commented Oct 8, 2014

I've ran into a problem that sounds like the very same thing in 1.7.12.
The symptoms in jstack output seem to point to the same issue: https://gist.github.com/jmiettinen/73e3ef5965312b5ae152

@cky
Copy link
Contributor Author

@cky cky commented Oct 8, 2014

The problem still persists in 1.7.16. :-(

@headius
Copy link
Member

@headius headius commented Feb 26, 2015

I never saw this bug :-( We will definitely fix this.

@headius
Copy link
Member

@headius headius commented Feb 26, 2015

A short term fix by using a per-runtime lock instead of one per proxy class. This will serialize booting of these classes, but that may not be a real concern (this booting should usually happen before threads spin up anyway).

https://gist.github.com/headius/2aec24578482427e88d9

If this passes tests I will chat with @enebo about improving it and whether we should go with the quick fix in next releases.

headius added a commit that referenced this issue Feb 26, 2015
This is a temporary measure to avoid deadlocks when multiple
threads load mutually-dependent Java classes. We should replace
this with some sort of atomic update of the proxy classes.

Temporary fix for #1621.
@headius
Copy link
Member

@headius headius commented Feb 26, 2015

I've committed the temporary measure to 1.7 HEAD for release in 1.7.20. Will work to get a lock-free version put together.

@headius headius added this to the JRuby 1.7.20 milestone Feb 26, 2015
headius added a commit that referenced this issue Feb 26, 2015
This commit makes several substantial changes:

* JavaClass does not reference the associated proxy class; this
  now happens entirely through ClassValue logic.
* ClassValue logic for the proxy class checks for uninitialized
  proxy in current thread, otherwise proceeding to creation. First
  thread wins, but if any two actually start building at the same
  time they will do the same work and one will be thrown away.
  This allows us to eliminate locking.
* The "uninitialized" proxy reference now lives as a thread-local
  ClassValue.

The basic logic for booting a class goes like this:

* Request proxy from ClassValue proxy cache. If it's there, done.
* Check for thread-local ClassValue unfinished proxy. If it's
  there, we're in a thread that's binding classes and return it.
* Proceed to construct the proxy. Two threads may get to this
  point, but only the first one to complete its hierarchy will
  permanently cache the proxy.

This appears to pass all our tests and passes tests for #1621.
headius added a commit that referenced this issue Feb 27, 2015
* Java.getProxyClass is the one true way to get the Ruby proxy
  class for a Java class.
* Unified more of class vs interface proxy logic.
* Both #1621 and #2014 appear to be working with this commit.
kares added a commit to kares/jruby that referenced this issue Mar 2, 2015
…-class initialization
kares added a commit to kares/jruby that referenced this issue Mar 2, 2015
@headius
Copy link
Member

@headius headius commented Apr 30, 2015

Ultimately I'm not sure a single lock is actually going to work. Different threads could traverse a hierarchy of classes in a different order, no matter how we slice it. Multiple locks may always lead to deadlock potential for this logic.

What we can do is make sure the lock goes away quickly and isn't used for subsequent accesses. Much of that has been done in the by using a synchronized ClassValue; once the proxy has been created, it should be a direct lock-free access from then on.

We've also done a lot of cleanup of the proxy initialization logic, so I think we can call this issue fixed.

Perhaps those of you using Java integration heavily could check for contention in proxy creation and file issues if you see many threads waiting to create proxies.

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

Successfully merging a pull request may close this issue.

None yet
3 participants