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.

Sign up for GitHub

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

Jump to bottom

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
Labels
java integration JRuby 1.7.x
Milestone
JRuby 1.7.20

Comments

@cky
Copy link
Contributor

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 commented Apr 15, 2014

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

@jmiettinen
Copy link
Contributor

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 commented Oct 8, 2014

The problem still persists in 1.7.16. :-(

@headius
Copy link
Member

headius commented Feb 26, 2015

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

@headius
Copy link
Member

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
@headius
Use a single global lock for proxy creation to avoid deadlocks.
1ad3c67 
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 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
@headius
Remove JavaClass from participation in proxy binding.
5d4a39d 
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
@headius
Cleanup, reduce duplication, refactor methods for proxy creation.
79f0889 
* 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
@kares
add a test (from jruby#1621) for guarding against dead-locks in proxy…
fd8d921 
…-class initialization
kares added a commit to kares/jruby that referenced this issue Mar 2, 2015
@kares
add a test (from jruby#1621) for guarding against dead-locks in proxy…
61acccd 
…-class initialization
@headius
Copy link
Member

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.

@headius headius closed this as completed Apr 30, 2015
@kares kares mentioned this issue Jul 28, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
java integration JRuby 1.7.x
Projects
None yet
Milestone
JRuby 1.7.20
Development

No branches or pull requests
3 participants
@headius @cky @jmiettinen