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
