On 13 feb 2014, at 10:17, Jaroslav Bachorik <jaroslav.bacho...@oracle.com>
wrote:
> Hi Staffan,
>
> On 12.2.2014 18:27, Staffan Larsen wrote:
>> 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.
>
> Yes, bringing the VM to a stable state before calling other JDI functions
> helps to stabilize the test even without the additional synchronization via
> stdout/stdin.
>
> I just wonder whether this check should not be done inside
> com.sun.jdi.connect.LaunchingConnector#launch() implementation. Does it even
> make sense to hand off an unstable VM?
Good question, but hard to change now - all implementations depend on the
current functionality. The VMStartEvent also gives you a reference to the main
thread.
/S
>
> -JB-
>
>>
>> /Staffan
>>
>> On 12 feb 2014, at 18:04, shanliang <shanliang.ji...@oracle.com> 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 <shanliang.ji...@oracle.com> 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 <shanliang.ji...@oracle.com>
>>>>>>>>>> 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
>>>>>>>>>>>
>>>>>>>>>
>>>>
>>>
>>>
>>
>>
>
