I’m still not happy with this fix since I think the extra output stream synchronization logic is not needed - the debuggee should be suspended at all the interesting points. The fix I proposed is cleaner and (as far as I can tell) also fixes the problem. The only thing is that I can’t quite explain what goes wrong without the fix… I’d really like to understand that. I’ll try to dig deeper and see if I can understand exactly what happens.
/Staffan On 12 feb 2014, at 18:04, shanliang <[email protected]> wrote: > Staffan Larsen wrote: >> >> I think what you need to do is wait for the VMStartEvent before you add >> requests to the VM. Note this paragraph from the VirtualMachine doc: >> >> Note that a target VM launched by a launching connector is not >> guaranteed to be stable until after the VMStartEvent has been >> received. >> > I may miss something here, I believe VMStartEvent must be the first event, > when the test got ClassPrepareEvent, it must already received VMStartEvent. >> >> I think adding code that looks something like this will make the test stable: >> >> VirtualMachine vm = launchTarget(CLASS_NAME); >> EventQueue eventQueue = vm.eventQueue(); >> >> boolean started = false; >> while(!started) { >> EventSet eventSet = eventQueue.remove(); >> for (Event event : eventSet) { >> if (event instanceof VMStartEvent) { >> started = true; >> } >> if (event instanceof VMDeathEvent >> || event instanceof VMDisconnectEvent) { >> throw new Error("VM died before it started...:"+event); >> } >> } >> } >> >> System.out.println("Vm launched"); >> > The code you proposed could improve the test, it made sure that > TestPostFieldModification was started, but I am afraid that it did not > address the issue causing the failure, the issue I believe was that > TestPostFieldModification exited before or during FieldMonitor called > addFieldWatch(), that was why addFieldWatch() received > VMDisconnectedException. When the test was treating ClassPrepareEvent, even > if VMDeathEvent or VMDisconnectEvent arrived, it must be still waiting in the > eventQueue because it arrived after ClassPrepareEvent. > > My fix was to not allow TestPostFieldModification to exit before > addFieldWatch() was done. >> >> >> There is also no reason to call addFieldWatch() before the ClassPrepareEvent >> has been received. The call to vm..classesByName() will just return an empty >> list anyway. >> > I do not know why the test called addFieldWatch before ClassPrepareEvent had > been received, but yes the returned list was empty, so agree to remove it. >> While you are in there you can also remove the unused StringBuffer near the >> top of main(). >> > Yes it was already removed in version 01 > > Here is the new webrev: > http://cr.openjdk.java.net/~sjiang/JDK-8007710/02/ > > Thanks, > Shanliang >> >> Thanks, >> /Staffan >> >> On 11 feb 2014, at 18:30, shanliang <[email protected]> wrote: >> >> >>> Here is the new fix in which FieldMonitor will write to >>> TestPostFieldModification, to inform the latter to quit, as suggested bu >>> Jaroslav >>> http://cr.openjdk.java.net/~sjiang/JDK-8007710/01/ >>> >>> Thanks, >>> Shanliang >>> >>> shanliang wrote: >>> >>>> shanliang wrote: >>>> >>>>> Jaroslav Bachorik wrote: >>>>> >>>>>> On 11.2.2014 16:31, shanliang wrote: >>>>>> >>>>>>> Staffan Larsen wrote: >>>>>>> >>>>>>>> Hi Shanliang, >>>>>>>> >>>>>>>> I can’t quite see how the test can fail in this way. When the >>>>>>>> ClassPrepareEvent happens, the debuggee will be suspended. So when >>>>>>>> addFieldWatch() is called, the debuggee should not have moved. >>>>>>>> >>>>>>> I am not expert of jdi so I may miss something here. I checked the >>>>>>> failure trace and saw the report exception happen when FieldMonitor >>>>>>> received ClassPrepareEvent and was doing addFieldWatch. FieldMonitor did >>>>>>> call "vm.resume()" before treating events. >>>>>>> >>>>>> AFAICS, calling vm.resume() results in an almost immediate debuggee >>>>>> death. The gc() invoking thread "d" is flagged as a deamon and as such >>>>>> doesn't prevent the process from exiting. The other thread is not a >>>>>> daemon but will finish in only few cycles. >>>>>> >>>>> I looked at the class com.sun.jdi.VirtualMachine, here is the Javadoc of >>>>> the method "resume": >>>>> /** >>>>> * Continues the execution of the application running in this >>>>> * virtual machine. All threads are resumed as documented in >>>>> * {@link ThreadReference#resume}. >>>>> * >>>>> * @throws VMCannotBeModifiedException if the VirtualMachine is >>>>> read-only - see {@link VirtualMachine#canBeModified()}. >>>>> * >>>>> * @see #suspend >>>>> */ >>>>> void resume(); >>>>> My understanding is that the debuggee resumes to work after this call, >>>>> instead to die? >>>>> >>>> In fact the problem is here, the vm (TestPostFieldModification) should not >>>> die before FieldMonitor finishes addFieldWatch. >>>> >>>> Shanliang >>>> >>>>>>> I reproduced the bug by add sleep(1000) after vm.resume() but before >>>>>>> calling eventQueue.remove(); >>>>>>> >>>>>> It looks like some kind of synchronization between the debugger and the >>>>>> debuggee is necessary. But I wonder if you should better use the >>>>>> process.getOuptuptStream() to write and flush a message for the debugee >>>>>> indicating that it can exit. And in the debugee you would just do >>>>>> System.in.read() as the last statement in the main() method. Seems more >>>>>> robust than involving files. >>>>>> >>>>> It could work, but creating a file in the testing directory should have >>>>> no issue, but yes maybe less performance. >>>>> >>>>> Thanks, >>>>> Shanliang >>>>> >>>>>> Cheers, >>>>>> >>>>>> -JB- >>>>>> >>>>>> >>>>>>> Thanks, >>>>>>> Shanliang >>>>>>> >>>>>>>> One problem I do see with the test is that it does not wait for a >>>>>>>> VMStartEvent before setting up requests. I’m not sure if that could >>>>>>>> cause the failure in the bug report, though. >>>>>>>> >>>>>>>> /Staffan >>>>>>>> >>>>>>>> On 11 feb 2014, at 15:13, shanliang <[email protected]> wrote: >>>>>>>> >>>>>>>> >>>>>>>>> Hi , >>>>>>>>> >>>>>>>>> The problem could be that FieldMonitor did not have enough time to >>>>>>>>> "addFieldWatch" but the vm to monitor (TestPostFieldModification) was >>>>>>>>> already ended. >>>>>>>>> >>>>>>>>> So we should make sure that TestPostFieldModification exits after >>>>>>>>> FieldMonitor has done necessary. The solution proposed here is that >>>>>>>>> FieldMonitor creates a file after adding field watching, and >>>>>>>>> TestPostFieldModification quits only after finding the file. >>>>>>>>> >>>>>>>>> web: >>>>>>>>> http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/ >>>>>>>>> >>>>>>>>> bug: >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8007710 >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Shanliang >>>>>>>>> >>>>>>> >> > >
