RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Coleen Phillimore]

On 6/28/16 2:18 PM, Kim Barrett wrote:
>> On Jun 28, 2016, at 12:04 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> I like all the removal of the pending-list-locker related code, but can't really comment on the details of how it was replaced and interacts with all the GC code. The interface to the Reference class is fine, it is the GC code that pushes into the reference queue that I can't really comment on.
> Thanks David.
>
>> I have a couple of queries following up from Coleen's and Dan's reviews.
>>
>> First, the synchronization of the pending-reference-list leaves me somewhat perplexed. As Coleen noted you require the heap_lock to be held but also use an Atomic::xchg. You have a comment:
>>
>> +   // owns the lock.  Swap is used by parallel non-concurrent reference
>> +   // processing threads, where some higher level controller owns
>> +   // Heap_lock, so requires the lock is locked, but not necessarily by
>> +   // the current thread.
>>
>> Can you clarify the higher-level protocol that is at play there. Asserting that "someone" owns a lock seems only marginally useful if you can't be sure who that owner is, or when they might release it. Some more direct detecting of being within this higher-level protocol would be better, if it is possible to detect.
> Was Per’s explanation sufficient?
>
>> As previously mentioned the change to the initialization order is a concern to me - there are always unexpected consequences of making such changes, no matter how straight-forward they seem at the time. Pre-initialization of exception classes is not really essential as the attempt to throw any of them from Java code will force initialization anyway - it's only for exceptions created and thrown from the VM code that direct initialization is needed. The problematic case is OOME because throwing the OOME may trigger a secondary OOME during that class initialization etc. Hence we try to deal with that by pre-allocating instances that do not have stacktraces etc, so they can be thrown safely. Hence my earlier comment that I think the real bug in your situation is that gen_out_of_memory_error() is not considering how far into the VM init sequence we are, and that it should be returning the pre-allocated instance with no backtrace.
> I'm also wondering why the existing order must have worked before the
> pre-initialization of InterruptedException by Reference, but now
> doesn't when we take that out.  I'll see if I can figure out what's
> going on there.  It might be some bug was introduced but was being
> masked by the Reference initialization.

Regarding the initialization order.  It would be interesting to know 
what bug you hit, because it looks like this call_initPhase1() function 
was added for modules to break up java.lang.System initialization.  It 
was probably added in this place because it initialized class Property, 
and java/lang/Throwable.<init> used to query a property.   This was 
taken out so it is safe to move the new initPhase1() initialization later.

Exceptions, especially OOM should be initialized very early, especially 
since OOM has preallocated stack traces.  It wouldn't surprise me if it 
needed to be moved further up someday.   All this depends on future JDK 
changes though.

Again, I think this looks fine.  I believe your testing would have found 
if there was anything wrong with this.

Coleen


>
>> ---
>>
>> A query on the JDK side:
>>
>> src/java.base/share/classes/java/lang/ref/Reference.java
>>
>> private static native Reference<? super Object> popReferencePendingList(boolean wait);
>>
>> I'm intrigued by the use of the lower-bounded wildcard using Object. As Object has no super-types this looks very strange and should be equivalent to the more common upper-bounded wildcard using Object ie Reference<? extends Object> or more concisely Reference<?>. I see this construct exists in the current code for the ReferenceQueue, but I am quite intrigued as to why? (Someone getting ready for Any-types? :) )
> I botched that. I meant to have a real Java programmer (which I don't
> qualify as) look at that part of the change before sending it out for
> review, but seem to have forgotten to do so. And lulled into a false
> sense of security, since neither the compiler nor the runtime
> complained, unlike the reams of template instantiation error output or
> segfaults at runtime that I'd expect with C++...
>