Hi Leonid,
Updated webrev version with the suggested refactoring is:
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:
Updated webrev version with the suggested refactoring is:
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.
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.
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.
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
