[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: [Fwd: Deadlocked Thread State is RUNNABLE?]
From: dmytro_sheyko () hotmail ! com (Dmytro Sheyko)
Date: 2009-11-25 13:26:33
Message-ID: BLU142-W2503F7B6B720B7CD643ABF8A9C0 () phx ! gbl
[Download RAW message or body]
Thank you, David.
I agree with you that proposed change would lead to race condition. I wonder, is this \
possible to avoid deadlocks in class initialization without sacrifice to performance?
As for poor design, sometimes base class may cause its subclass initialization.
For example,
enum Base {
DERIVED {};
}
Of cource, it's safe in the case of possible deadlocks. But anyway, I believe that \
runtime should get round possible problems, if it's possible and not hard to do. Even \
if it is really bad design, it shouldn't worsen the situation falling into deadlock.
> Date: Wed, 25 Nov 2009 10:41:23 +1000
> From: David.Holmes at Sun.COM
> Subject: Re: [Fwd: Deadlocked Thread State is RUNNABLE?]
> To: dmytro_sheyko at hotmail.com
> CC: serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
>
> Dmytro,
>
> Okay I see what you mean. However I'm more inclined to go with a design
> that benefits normal correct usage than to avoid a deadlock caused by
> poor design: a Base class should never depend on its subclasses during
> initialization.
>
> With the change you suggest, two threads trying to initialize the same
> class will race each other to the top of the hierarchy (or the first
> uninitialized class anyway) where one will perform the clinit and the
> other will wait. When clinit is done the waiting thread will be
> signalled and wakeup, both threads then race to initialize the next
> subclass - one will win and one will wait ... this process then
> continues down the hierarchy with the two threads "butting heads" at
> every step of the way. This is potentially a very wasteful process.
>
> Of course I can't speak to the original design and what considerations
> were made at that time - I wasn't there. This is just my opinion.
>
> Cheers,
> David Holmes
>
> Dmytro Sheyko said the following on 11/24/09 23:00:
> > David,
> >
> > > The lock of the class is not held during its own or its superclass's
> >
> > Yes, I know. I mean, let's say, artifical lock that prevents
> > simultaneous class initialization from different threads.
> >
> > The current scheme of class initialization is following:
> >
> > 1. Synchronize on the |Class| object that represents the class or
> > interface to be initialized. This involves waiting until the
> > current thread can obtain the lock for that object (?8.13)
> > <file:///C:/Tools/Java/docs/vmspec.2nded/Threads.doc.html#22500>.
> > 2. If initialization by some other thread is in progress for the
> > class or interface, then |wait| on this |Class| object (which
> > temporarily releases the lock). When the current thread awakens
> > from the |wait|, repeat this step.
> > 3. If initialization is in progress for the class or interface by the
> > current thread, then this must be a recursive request for
> > initialization. Release the lock on the |Class| object and
> > complete normally.
> > 4. If the class or interface has already been initialized, then no
> > further action is required. Release the lock on the |Class| object
> > and complete normally.
> > 5. If the |Class| object is in an erroneous state, then
> > initialization is not possible. Release the lock on the |Class|
> > object and throw a |NoClassDefFoundError|.
> > 6. Otherwise, record the fact that initialization of the |Class|
> > object is now in progress by the current thread and release the
> > lock on the |Class| object.
> > 7. Next, if the |Class| object represents a class rather than an
> > interface, and the direct superclass of this class has not yet
> > been initialized, then recursively perform this entire procedure
> > for the uninitialized superclass. If the initialization of the
> > direct superclass completes abruptly because of a thrown
> > exception, then lock this |Class| object, label it erroneous,
> > notify all waiting threads, release the lock, and complete
> > abruptly, throwing the same exception that resulted from the
> > initializing the superclass.
> > 8. Next, execute either the class variable initializers and static
> > initializers of the class or the field initializers of the
> > interface, in textual order, as though they were a single block,
> > except that |final| |static| variables and fields of interfaces
> > whose values are compile-time constants are initialized first.
> > 9. If the execution of the initializers completes normally, then lock
> > this |Class| object, label it fully initialized, notify all
> > waiting threads, release the lock, and complete this procedure
> > normally.
> > 10. Otherwise, the initializers must have completed abruptly by
> > throwing some exception E. If the class of E is not |Error| or one
> > of its subclasses, then create a new instance of the class
> > > ExceptionInInitializerError|, with E as the argument, and use
> > this object in place of E in the following step. But if a new
> > instance of |ExceptionInInitializerError| cannot be created
> > because an |OutOfMemoryError| occurs, then instead use an
> > > OutOfMemoryError| object in place of E in the following step.
> > 11. Lock the |Class| object, label it erroneous, notify all waiting
> > threads, release the lock, and complete this procedure abruptly
> > with reason E or its replacement as determined in the previous step.
> >
> >
> >
> > In this case if some thread started initialization of class DerivedA no
> > other thread can execute DerivedA.<clinit>.
> > I have following scheme in my mind (superclass initialization is moved up):
> >
> > 1. If the |Class| object represents a class rather than an interface,
> > and the direct superclass of this class has not yet been
> > initialized, then recursively perform this entire procedure for
> > the uninitialized superclass.
> > 2. Synchronize on the |Class| object that represents the class or
> > interface to be initialized. This involves waiting until the
> > current thread can obtain the lock for that object (?8.13)
> > <file:///C:/Tools/Java/docs/vmspec.2nded/Threads.doc.html#22500>.
> > 3. If initialization by some other thread is in progress for the
> > class or interface, then |wait| on this |Class| object (which
> > temporarily releases the lock). When the current thread awakens
> > from the |wait|, repeat this step.
> > 4. If initialization is in progress for the class or interface by the
> > current thread, then this must be a recursive request for
> > initialization. Release the lock on the |Class| object and
> > complete normally.
> > 5. If the class or interface has already been initialized, then no
> > further action is required. Release the lock on the |Class| object
> > and complete normally.
> > 6. If the |Class| object is in an erroneous state, then
> > initialization is not possible. Release the lock on the |Class|
> > object and throw a |NoClassDefFoundError|.
> > 7. Otherwise, record the fact that initialization of the |Class|
> > object is now in progress by the current thread and release the
> > lock on the |Class| object.
> > 8. Next, execute either the class variable initializers and static
> > initializers of the class or the field initializers of the
> > interface, in textual order, as though they were a single block,
> > except that |final| |static| variables and fields of interfaces
> > whose values are compile-time constants are initialized first.
> > 9. If the execution of the initializers completes normally, then lock
> > this |Class| object, label it fully initialized, notify all
> > waiting threads, release the lock, and complete this procedure
> > normally.
> > 10. Otherwise, the initializers must have completed abruptly by
> > throwing some exception E. If the class of E is not |Error| or one
> > of its subclasses, then create a new instance of the class
> > > ExceptionInInitializerError|, with E as the argument, and use
> > this object in place of E in the following step. But if a new
> > instance of |ExceptionInInitializerError| cannot be created
> > because an |OutOfMemoryError| occurs, then instead use an
> > > OutOfMemoryError| object in place of E in the following step.
> > 11. Lock the |Class| object, label it erroneous, notify all waiting
> > threads, release the lock, and complete this procedure abruptly
> > with reason E or its replacement as determined in the previous step.
> >
> >
> > Consider example:
> > class Base {
> > static {
> > DerivedB.init();
> > }
> > }
> > class DerivedA extends Base {}
> > class DerivedB extends Base {}
> >
> > ThreadA initializes DerivedA
> > ThreadB initializes DerivedB
> >
> > Existing scheme scenario:
> > 1. ThreadA marks DerivedA as being initialized by it (i.e. ThreadA).
> > 2. ThreadB marks DerivedB as being initialized by it (i.e. ThreadB).
> > 3. ThreadA starts initialization of class Base.
> > 4. ThreadB attempts to initialize class Base and waits for ThreadA.
> > 5. ThreadA invokes Base.<clinit>()
> > 6. ThreadA attempts to initialize class DerivedB and waits for ThreadB
> > (triggered by static method call).
> > --> deadlock.
> >
> > New scheme scenario:
> > 1. ThreadA starts initialization of class Base (superclass of DerivedA);
> > 2. ThreadB attempts to initialize class Base and waits for ThreadA.
> > 3. ThreadA invokes Base.<clinit>()
> > 4. ThreadA starts initialization of class DerivedB (triggered by static
> > method call)
> > 5. ThreadA starts initialization of class Base and it figures out that
> > is already being initialized by it - normal completion.
> > 6. ThreadA marks DerivedB as initialized;
> > 7. ThreadA marks Base as initialized;
> > 8. ThreadB wakes up and figures out that Base is already initialized;
> > 9. ThreadB figures out that DerivedB is already initialized;
> > 10. ThreadA marks DerivedA as initialized;
> >
> > --
> > Dmytro
> >
> > > Date: Tue, 24 Nov 2009 20:05:26 +1000
> > > From: David.Holmes at Sun.COM
> > > Subject: Re: [Fwd: Deadlocked Thread State is RUNNABLE?]
> > > To: dmytro_sheyko at hotmail.com
> > > CC: serviceability-dev at openjdk.java.net;
> > hotspot-runtime-dev at openjdk.java.net
> > >
> > > Dmytro Sheyko said the following on 11/24/09 19:42:
> > > > What is the reason to hold lock on class being initialized while
> > > > initializing its superclasses?
> > >
> > > The lock of the class is not held during its own or its superclass's
> > > initialization. A lock is only used to set the current class as
> > > being-initialized, and for concurrent initialization attempts to be able
> > > to wait.
> > >
> > > David Holmes
> > >
> > > > Wouldn't it better acquire lock after superclass initialization just
> > > > before calling static initializer?
> > > > This would prevents some kind of deadlocks.
> > > >
> > > > Thanks,
> > > > Dmytro
> > > >
> > > > > Date: Wed, 18 Nov 2009 11:58:18 +1000
> > > > > From: David.Holmes at Sun.COM
> > > > > Subject: Re: [Fwd: Deadlocked Thread State is RUNNABLE?]
> > > > > To: Mandy.Chung at Sun.COM
> > > > > CC: serviceability-dev at openjdk.java.net;
> > > > hotspot-runtime-dev at openjdk.java.net
> > > > >
> > > > > Mandy Chung said the following on 11/18/09 11:36:
> > > > > > It's a known bug:
> > > > > >
> > > > > > 6501158: Thread state is incorrect during class initialization
> > > > > > procedure
> > > > > >
> > > > > > I recalled the discussion for this bug but don't remember if we
> > > > > > discussed enhancing the java.lang.management spec to cover
> > "waiting"
> > > > > > on VM internal actions.
> > > > > >
> > > > > > David will probably have more information about this.
> > > > >
> > > > > I have nothing really to add save what is stated in the CR, but as my
> > > > > main comment was not public I've moved it to being public (and
> > dropped
> > > > > myself as RE) and reproduce it below.
> > > > >
> > > > > Quite simply the code that does the "wait" is low-level in the VM and
> > > > > does not come through the normal Object.wait() path that would
> > set the
> > > > > Thread.State. It can be "fixed" but there are a couple of additional
> > > > > issues that also need to be addressed due to the fact that the
> > monitor
> > > > > used is not associated with Java-level object. (The JLS was
> > updated in
> > > > > this regard.)
> > > > >
> > > > > The meta-discussion was whether we should introduce a new
> > Thread.State
> > > > > to cover this special case (waiting for class initialization),
> > and that
> > > > > discussion seemed to lean towards doing this (I suggested it and
> > Mandy
> > > > > agreed it seemed like a good idea :) ) But things did not
> > progress from
> > > > > there.
> > > > >
> > > > > Cheers,
> > > > > David
> > > > > -----
> > > > >
> > > > > > From 6501158:
> > > > >
> > > > > The submitter quotes the JLS with regard to the class initialization
> > > > > procedure and how synchronization is employed. In fact hotspot
> > does not
> > > > > synchronize using the Class object monitor during class
> > initialization -
> > > > > this is to avoid denial-of-service style attacks just by explicitly
> > > > > locking a Class object. The JLS is in the process of being updated to
> > > > > say that a "unique initialization lock " is used for class
> > > > > initialization, not necessarily the Class object's lock. This
> > brings the
> > > > > spec into line with the hotspot implementation.
> > > > >
> > > > > The reason I mention this is that the monitor that hotspot uses is
> > > > > associated with the klassOop for the class. The monitor code sets
> > > > > current_waiting_monitor() or current_pending_monitor() as appropriate
> > > > > during wait() or monitor entry. The management code, via the
> > > > > ThreadService::ThreadSnapShot gets a hold of the object
> > associated with
> > > > > the monitor for a blocked thread and assumes that the object is
> > in fact
> > > > > the oop for a java.lang.Object. When the klassOop is treated as in
> > > > > instance oop and queried for its own class etc then we end up
> > crashing
> > > > > the VM.
> > > > >
> > > > > The suggested fix correctly sets the thread state to "WAITING":
> > > > >
> > > > > Full thread dump Java HotSpot(TM) Tiered VM
> > > > > (1.7.0-internal-dh198349-fastdebug mixed mode):
> > > > >
> > > > > "Runner" prio=3 tid=0x08171800 nid=0xb in Object.wait()
> > > > > [0xcb99d000..0xcb99dbb0]
> > > > > java.lang.Thread.State: WAITING (on object monitor)
> > > > >
> > > > > but additional changes are need in ThreadSnapShot to discard the
> > > > > non-instance oop. (It seems
> > JvmtiEnvBase::get_current_contended_monitor
> > > > > would need a similar modification). This seems to work and
> > getThreadInfo
> > > > > simply reports eg:
> > > > >
> > > > > Current thread info: "Runner" Id=8 WAITING
> > > > >
> > > > > which seems okay. And getLockInfo() returns null.
> > > > >
> > > > > It is unclear however whether reporting this information actually
> > > > > violates the specification for these management API's. A thread
> > is only
> > > > > WAITING when performing Object.wait(), in which case there must be an
> > > > > Object being waited upon and so LockInfo must return non-null
> > > > > information. Yet that is not the case here.
> > > > >
> > > > > It seems to me that while we can report the information above, it
> > might
> > > > > be better to see whether the management specification can be
> > enhanced to
> > > > > cover "waiting" on VM internal actions and to then report this
> > > > > circumstance as one of those.
> > > > >
> > > > > Note also that the existing hotspot code could already be
> > susceptible to
> > > > > a crash due to the use of the klassOop monitor for class
> > initialization.
> > > > > If the timing were just right, a call to getThreadInfo could see a
> > > > > thread blocked trying to acquire this monitor (not wait upon it) and
> > > > > that would be captured by the ThreadSnapshot and eventually cause a
> > > > > crash. The fact that the snapshot requires a safepoint makes it less
> > > > > likely that you would encounter the target thread while blocked
> > on the
> > > > > monitor, as the monitor is only held for a short period during class
> > > > > initialization.
> > > > >
> > > > > I will await discussion with the management/monitoring folk before
> > > > > deciding how best to proceed with this CR.
> > > > >
> > > >
> > > >
> > ------------------------------------------------------------------------
> > > > Windows Live: Keep your friends up to date with what you do online.
> > > >
> > <http://www.microsoft.com/middleeast/windows/windowslive/see-it-in-action/social-network-basics.aspx?ocid=PID23461::T:WLMTAGL:ON:WL:en-xm:SI_SB_1:092010>
> >
> > ------------------------------------------------------------------------
> > Windows Live Hotmail: Your friends can get your Facebook updates, right
> > from Hotmail?.
> > <http://www.microsoft.com/middleeast/windows/windowslive/see-it-in-action/social-network-basics.aspx?ocid=PID23461::T:WLMTAGL:ON:WL:en-xm:SI_SB_4:092009>
> >
_________________________________________________________________
Windows Live: Make it easier for your friends to see what you?re up to on Facebook.
http://www.microsoft.com/middleeast/windows/windowslive/see-it-in-action/social-network-basics.aspx?ocid=PID23461::T:WLMTAGL:ON:WL:en-xm:SI_SB_2:092009
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20091125/32be8185/attachment-0001.html \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic