Hi Dmitry, On Fri, 2016-04-15 at 21:59 +0300, Dmitry Samersoff wrote: > Severin, > > Looks good for me.
Thanks for the review! > But I'm a little afraid of the fact that now we are holding > eventHandler_lock while doing invoke*. OK. FWIW, we need to hold this lock for proper lock ordering. Otherwise com/sun/jdi/InvokeHangTest.java fails: diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c b/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c --- a/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c +++ b/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c @@ -670,7 +670,6 @@ return JNI_FALSE; } - eventHandler_lock(); /* for proper lock order */ debugMonitorEnter(invokerLock); env = getEnv(); @@ -706,7 +705,6 @@ } END_WITH_LOCAL_REFS(env); debugMonitorExit(invokerLock); - eventHandler_unlock(); > So please hold on with backports until the fix bakes in jdk9 for some > time. Sounds good. Sorry for the trouble last time around. Cheers, Severin > -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. > > > > 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/we > > brev.01/ > > > > Once reviewed, I'd need somebody to sponsor this patch. > > > > Thanks, > > Severin > > >
