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, [email protected] 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
