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. 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 > > > > > > > > > >
