On 4/19/16 02:16, Severin Gehwolf wrote:
Hi,
On Tue, 2016-04-19 at 01:33 -0700, serguei.spit...@oracle.com wrote:
Hi Severin,
Please, find my comments below.
On 4/15/16 13:52, serguei.spit...@oracle.com wrote:
On 4/15/16 11:59, Dmitry Samersoff wrote:
Severin,
Looks good for me.
But I'm a little afraid of the fact that now we are holding
eventHandler_lock while doing invoke*.
It seems, I have this concern too.
Please, let me take a look closer at this part if it is done in a right way.
Ok, my question was not about the eventHandler_lock, but about the invokerLock. Please, see below.
So please hold on with backports until the fix bakes in jdk9 for some time.
+1
Thanks,
Serguei
-Dmitry
On 2016-04-15 19:53, Severin Gehwolf wrote:
Hi,
Here is a patch which is a redo of the fix for JDK-4858370 which got
backed out due to it causing test regressions. Specifically problems
were reported for com/sun/jdi/InvokeTest.java
and com/sun/jdi/InterfaceMethodsTest.java with the fix for JDK-4858370
applied.
Those test regressions were caused because:
a.) The fix for JDK-4858370 deleted refs from the request object
while *not* holding the invoker lock.
b.) Invokes done via invoker_doInvoke() save global references, but
don't hold the invoker lock either.
It seems, the invokerLock is to protect any uses of the 'request' pointer that points
to the field ThreadNode.currentInvoke, not to protect the saveGlobalRef() call
itself.
So that, we have a hole in synchronization that you nicely discovered.
Yes, that's right. saveGlobalRef() saves references by using the request
pointer.
Here is a couple of more places where the 'request' pointer is not protected
with
the invokerLock:
invoker_enableInvokeRequests(), invoker_isPending(), invoker_isEnabled().
I've filed a new bug:
https://bugs.openjdk.java.net/browse/JDK-8154529
BTW, these are pretty simple or rare cases that do not harm much.
The function invoker_isPending() is not used at all.
OK.
Please, consider your fix reviewed.
Thanks!
The hg exported changeset is here:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/JDK-8153711-jdk9-jdk.export.patch
I'd need somebody to sponsor this for me.
I will sponsor the fix for you.
Thanks,
Serguei
Cheers,
Severin
Thanks!
Serguei
This new fix grabs relevant locks for both cases above.
Testing done:
- com/sun/jdi test set. No regressions + added regression test.
- com/sun/jdi/InterfaceMethodsTest.java did not fail in 1300
invocations. Same for com/sun/jdi/InvokeTest.java.
- I haven't seen any crashes in new OomDebugTest.java either.
Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/
Once reviewed, I'd need somebody to sponsor this patch.
Thanks,
Severin
