RFR (L): 8241807: JDWP needs update for hidden classes
Alex Menkov
alexey.menkov at oracle.com
Wed Apr 29 00:35:46 UTC 2020
Hi Serguei,
Looks good in general.
couple minor notes:
DebuggerBase.java
- vm, debuggee, pipe and erManager fields and getDebuggee(), vm()
methods don't need to be static;
- replace all "imultiple" with "multiple".
--alex
On 04/27/2020 18:34, serguei.spitsyn at oracle.com wrote:
> Hi Leonid,
>
> Updated webrev version with the suggested refactoring is:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.5/
> <http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.5/>
>
> <http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.5/>
>
> Please, see my comments below.
>
> On 4/24/20 13:20, Leonid Mesnik wrote:
>>
>> Hi
>>
>> I have several comments about coding and desing. These tests might be
>> used as examples for new JDI tests based on this framework so I think
>> it would be better to polish them.
>>
>
> Yes, as we agreed it'd be nice to create good examples, a base for a
> future test development in the JDI area.
> I also wanted to polish these tests as much as possible, so thanks for
> helping with it.
> These are good suggestions from you below.
> One problem is that these tests will differ in style from the rest of
> the NSK JDI tests but it has to be okay.
>
>
>> General comment:
>>
>> 1. You often check results (not null, exactly one result).
>>
>> It makes sense to use jdk.test.lib.Asserts for such verification. Just
>> to reduce number of code and fail with standard exceptions.
>>
>
> Good suggestion - fixed now.
>
>> 2. Wildcard imports like 'import com.sun.jdi.*;' are not recommended.
>>
>
> Good suggestion - fixed.
> The list of imports became big but I see no problem with that.
>
>> 3. I recommend to catch Throwable instead of Exception especially in
>> non-main threads. Just to ensure that nothing is missed.
>>
>
> God suggestion - fixed, at least, in places where it makes sense to do.
>
>> 4. nsk.share.Failure is subclass of RuntimeException so you don't need
>> to add it to throws. Now sometimes it is added, sometimes not. It
>> looks inconsistent.
>>
>
> Good suggestion - fixed.
> In fact, I did not like the use of Failure as it creates this
> inconsistency, so I tried to get rid of it.
>
>> 5. There are lot of code duplication in debugger and debugge classes.
>> Does it make a sense to merge it into base classes?
>>
>
> I was thinking about this too but was reluctant to do so, as there are
> just two tests.
> But it has to be beneficial to move shared code to the JDI testing
> framework.
> I've just made some initial preparation by separating common parts
> DebuggeeBase and DebuggerBase.
> Also, it seems to be natural to move the HiddenClass and EventHandler
> classes to their own files.
> Then I've decided to combine both tests into one.
> All refactoring still makes sense to have.
>
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent/classPrepEvent001.java.html
>>
>> 6. The getField can't return null, it verifies result. So check in
>> line 191-193 is not needed.
>>
>> 190 Field field = getField(hcRefType, "hcField");
>> 191 if (field == null) {
>> 192 throw new Failure("TEST FAILED: cannot enable ModificationWhatchpointRequest: hcField not found");
>> 193 }
>
> Good catch - fixed.
>
>>
>> 7. I would recommend to move all initialization from run in
>> constructor and make most of variable 'final' it helps to ensure that
>> they are initialized.
>> 297 public int run(final String args[], final PrintStream out) {
>> 298 ArgumentHandler argHandler = new ArgumentHandler(args);
>> 299
>> 300 log = new Log(out, argHandler);
>> 301 eventTimeout = argHandler.getWaitTime() * 60 * 1000; // milliseconds
>> 302 launchDebuggee(argHandler);
>
> Right comment in general, but I did not go with a constructor and final
> fields.
> Instead, I've just reorganized this a little bit.
> There is a little base to refactor to the constructor because the field
> "log" needs to be static and timeout can be local.
>
>
>> 8. Pair looks inconsistent. start saves handler as class field while
>> join use as parameter.
>> 305 startEventHandler();
>> 330 joinEventHandler(eventHandler);
>>
>> I think that something like
>> EventHandler eventHandler = startEventHandler();
>> joinEventHandler(eventHandler);
>> looks better and restrict usage of eventHandler. You also migt move
>> methods startEventHandler into EventHandler itself.
>
> Good suggestion - fixed. Moved both methods to the EventHandler class.
>
>>
>> 9. I suggest to make setHCRefType private and rename getHCRefType to
>> something like receiveHCRefType. To make more clear that it is not a
>> simple getter, but method which wait for event.
>> 361 // Save hidden class ReferenceType when its ClassPrepare event is received.
>> 362 synchronized void setHCRefType(ReferenceType type) {
>> 363 hcRefType = type;
>> 364 notifyAll();
>> 365 }
>> 366
>> 367 // This method is used by the debugger main thread to wait and get
>> 368 // the hidden class reference type from its ClassPrepare event.
>> 369 // The readyCmdSync with the debuggeee is not enough because a
>> 370 // ClassPrepare event sent over JDWP protocol has some delay.
>> 371 // A wait/notify sync is to ensure the debugger gets non-null value.
>> 372 synchronized ReferenceType getHCRefType() {
>> 373 try {
>> 374 while (hcRefType == null) {
>> 375 wait();
>> 376 }
>> 377 } catch (InterruptedException ie) {
>> 378 throw new Failure("Unexpected InterruptedException in waiting for HC prepare: " + ie);
>> 379 }
>> 380 return hcRefType;
>> 381 }
>
> Good suggestion - fixed. I've replaced getHCRefType with waitGetHCRefType.
> Also, I've removed all uses of Failure and InterruptedException catches
> and specified a couple of methods to throw InterruptedException.
>
>>
>> 10. We don't run tests with -ea/esa so assertions usually disabled.
>> Use Assertions from test lib instead. The comment 1) is recommendation
>> only but 'assert' is just ignored.
>> 436 assert name.indexOf("HiddenClass") > 0 && name.indexOf("/0x") > 0;
>
> Thanks - fixed.
>
>>
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent/classPrepEvent001a.java.html
>>
>>
>> 11. Check if jdk.test.lib.classloader.ClassLoadUtils.
>> getClassPath(String className) works for you instead of
>> 70 private static String getClassPath(Class<?> klass) {
>>
>> 12. You could use try(log = new PrintStream("Debuggee.log")) {} here:
>> 82 log = new PrintStream("Debuggee.log");
>> ..
>> 86 try {
>> ..
>> 90 }
>> 91 log.close();
>
> Good idea, but unfortunately it does not work as the field needs to be
> static.
>
>
>> 13. It is better to throw RuntimeException and don't add throws
>> Exception in all methods.
>> 95 private void syncWithDebugger(IOPipe pipe) throws Exception {
>
> Good suggestion - fixed.
>
>
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassUnloadEvent/classUnloadEvent001a.java.html
>>
>> You could use single if for this.
>> 122 if (command == null) {
>> 123 throw new Exception("Failed: pipe.readln() returned null instead of COMMAND_QUIT");
>> 124 } else if (!command.equals(classUnloadEvent001.COMMAND_QUIT)) {
>> 125 throw new Exception("Failed COMMAND_QUIT sync with debugger, got unknown command: " + command);
>> 126 }
>
> I wanted a separate error message for (command == null) as it was
> related to OOME on the debuggee side.
> But it is not important anymore after the WT.fullGC() is used.
> Fixed.
>
>> 14. Setting to null is not required to collect objects here.
>> 151 hcObj = null;
>> 152 hc = null;
>
> It is better to keep these to be sure the hidden class can be unloaded.
> I had to create one mode hidden class to get a ClassUnload event.
> There is a comment in the test debuggee explaining it.
>
>
>> Probably I didn't catch all issues, but let discuss this list first.
>
> Okay.
>
> Thanks!
> Serguei
>
>> Leonid
>>
>> On 4/23/20 9:01 PM, serguei.spitsyn at oracle.com wrote:
>>> Please, review a fix for the sub-task:
>>> https://bugs.openjdk.java.net/browse/JDK-8241807
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/
>>>
>>>
>>> Summary;
>>> This sub-task is to cover any hidden class related remaining issues
>>> in the JDI and JDWP implementation.
>>> In fact, no more issues were discovered with this extra test coverage.
>>> So, this update includes only two new nsk jdi tests:
>>> || test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent
>>> || test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassUnloadEvent
>>>
>>> These tests are to provide a test coverage which was impossible to
>>> provide with the jdb testing framework.
>>> It includes ClassPrepare, Breakpoint and ModificationWatchpoint
>>> events with class filters.
>>>
>>> Testing:
>>> The mach5 job is in progress:
>>> https://mach5.us.oracle.com/mdash/jobs/sspitsyn-HiddenClass-jdi-event-tests-20200424-0350-10464032
>>>
>>> Thanks,
>>> Serguei
>
More information about the serviceability-dev
mailing list