On Tue, 2016-04-19 at 19:32 -0700, serguei.spit...@oracle.com wrote: > On 4/19/16 19:06, serguei.spit...@oracle.com wrote: > > > > On 4/19/16 02:22, serguei.spit...@oracle.com wrote: > > > > > > 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. > > Hi Severin, > > > > I postpone a push for this fix. > > > > There are two nsk.jdi test failures (they look like deadlocks): > > nsk/jdi/ObjectReference/invokeMethod/invokemethod012 FAIL(TIMEOUT) > > nsk/jdi/Scenarios/invokeMethod/popframes001 FAIL(TIMEOUT)
Those seem closed tests, no? At least I don't see those. Any pointers? > > and one jtreg sun/com/jdi failure (it looks like a deadlock too): > > com/sun/jdi/InvokeHangTes.java This seems one I can work with. On which platform does it fail? I've run this test > 100 times when I last tested the patch and didn't observe hangs. > > I'll double check if these failures are regressions caused by your fix > > or not. > I confirm, the failures above are new regressions introduced by the fix. > The tests fail consistently with the fix and do not fail without the fix. > This new fix grabs relevant locks for both cases above. Thanks for checking! I'll have another look. Hopefully I can reproduce. Cheers, Severin > > > > > > > > > > > > > > > > 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 > > > > > > > >
