Hi Yasumasa, Serguei and Alex, Please review a new version of the webrev that merges SADebugDTest.java with changes done in [2].
Also the CRS [3] and the help message for debug server in SALauncher.java were updated to specify that '--hostname' option could be a hostname or an IPv4/IPv6 address. > Ok, but I think it might be more simply with TestLibrary. > For example, can you use TestLibrary::getUnusedRandomPort ? It is used in > test/jdk/java/rmi/testlibrary/RMID.java . TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports are reserved and it uses some hardcoded port range [FIXED_PORT_MIN, FIXED_PORT_MAX] as reserved ports. Besides, test/jdk/java/rmi/testlibrary/TestLibrary.java class cannot be directly used in test/hotspot/jtreg/serviceability/* tests (it doesn't compile). Nevertheless, to simplify the test itself I moved findUnreservedFreePort(int .. reservedPorts) from SADebugTest.java to jdk.test.lib.Utils in /test/lib. Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. [1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/ [2] https://bugs.openjdk.java.net/browse/JDK-8238268 [3] https://bugs.openjdk.java.net/browse/JDK-8239831 Thank you, Daniil On 3/13/20, 7:23 PM, "Yasumasa Suenaga" <suen...@oss.nttdata.com> wrote: Hi Daniil, On 2020/03/14 7:05, Daniil Titov wrote: > Hi Yasumasa, Serguei and Alex, > > Please review a new version of the webrev that includes the changes Yasumasa suggested. > >> Shutdown hook is already registered in c'tor of HotSpotAgent. >> It works same as shutdownServer(), so I think shutdown hook at SALauncher is not needed. > > The shutdown hook registered in the HotSpotAgent c'tor only works for non-servers, so we still need a > the shutdown hook for remote server being added in SALauncher. I changed it to use the lambda expression. > > 101 public HotSpotAgent() { > 102 // for non-server add shutdown hook to clean-up debugger in case > 103 // of forced exit. For remote server, shutdown hook is added by > 104 // DebugServer. > 105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread( > 106 new Runnable() { > 107 public void run() { > 108 synchronized (HotSpotAgent.this) { > 109 if (!isServer) { > 110 detach(); > 111 } > 112 } > 113 } > 114 })); > 115 } I missed it, thanks! >>> Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains >>> `exclusiveAccess.dirs=.` to avoid concurrent execution > As I understand exclusiveAccess.dirs prevents only the tests located in this directory from being run simultaneously and other tests could still run in parallel with one of these tests. Thus I would prefer to have the retry mechanism in place. I simplified the code using the class variables instead of local arrays. Ok, but I think it might be more simply with TestLibrary. For example, can you use TestLibrary::getUnusedRandomPort ? It is used in test/jdk/java/rmi/testlibrary/RMID.java . Thanks, Yasumasa > Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/ > [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > [3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751 > > Thank you, > Daniil > > On 3/6/20, 6:15 PM, "Yasumasa Suenaga" <suen...@oss.nttdata.com> wrote: > > Hi Daniil, > > On 2020/03/07 3:38, Daniil Titov wrote: > > Hi Yasumasa, > > > > -> checkBasicOptions() is needed? I think you can remove this method and embed it in caller. > > I think that having a piece of code that invokes a method named "buildAttachArgs" with a copy of the argument map just for its side-effect ( it throws an exception if parameters are incorrect) and ignores its return might look confusing. Thus, I found it more appropriate to wrap it inside a method with more relevant name . > > Ok, but I prefer to leave comment it. > > > > > SADebugDTest > > > - Why do you declare portInUse and testResult as array? Their length is 1, so I think you don't need to use array. > > We cannot use primitives there since these local variables are captured in lambda expression and are required to be final. > > The other option is to use some other wrapper for them but I don't see any obvious benefits in it comparing to the array. > > Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains `exclusiveAccess.dirs=.` to avoid concurrent execution. > If you do not think this error check, test code is more simply. > > > > I will include your other suggestion in the new version of the webrev. > > Sorry, I have one more comment: > > > - Shutdown hook is very good idea. You can implement more simply if you use lambda expression. > > Shutdown hook is already registered in c'tor of HotSpotAgent. > It works same as shutdownServer(), so I think shutdown hook at SALauncher is not needed. > > > Thanks, > > Yasumasa > > > > Thanks! > > Daniil > > > > On 3/6/20, 12:30 AM, "Yasumasa Suenaga" <suen...@oss.nttdata.com> wrote: > > > > Hi Daniil, > > > > > > - SALauncher.java > > - checkBasicOptions() is needed? I think you can remove this method and embed it in caller. > > - I think registryPort should be checked with Integer.parseInt() like others (rmiPort and pid) rather than regex. > > - Shutdown hook is very good idea. You can implement more simply if you use lambda expression. > > > > - SADebugDTest.java > > - Please add bug ID to @bug. > > - Why do you declare portInUse and testResult as array? Their length is 1, so I think you don't need to use array. > > > > > > Thanks, > > > > Yasumasa > > > > > > On 2020/03/06 10:15, Daniil Titov wrote: > > > Hi Yasumasa, Serguei and Alex, > > > > > > Please review a new version of the fix [1] that addresses your comments. The new version in addition to RMI connector > > > port option introduces two more options to specify RMI registry port and RMI connector host name. Currently, these > > > last two settings could be specified using the system properties but the system properties have the following disadvantages > > > comparing to the command line options: > > > - It’s hard to know about them: they are not listed in tool’s help. > > > - They have long names that hard to remember > > > - It is easy to mistype them in the command line and you will not get any warning about it. > > > > > > The CSR [2] was also updated and needs to be reviewed. > > > > > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > > > container and connecting to it with the GUI debugger. Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > > > > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.02/ > > > [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > > [3] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > > > > > > Thank you, > > > Daniil > > > > > > On 2/24/20, 5:45 AM, "Yasumasa Suenaga" <suen...@oss.nttdata.com> wrote: > > > > > > Hi Daniil, > > > > > > - SALauncher::buildAttachArgs is not only to build arguments but also to check consistency of arguments. > > > Thus you should use buildAttachArgs() in runDEBUGD(). If you do so, runDEBUGD() would be more simply. > > > > > > - SADebugDTest::testWithPidAndRmiPort would retry until `--rmiport` can be used. > > > But you can use same port number as RMI registry (1099). > > > It is same as relation between jmxremote.port and jmxremote.rmi.port. > > > > > > > > > Thanks, > > > > > > Yasumasa > > > > > > > > > On 2020/02/24 13:21, Daniil Titov wrote: > > > > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > > > > Currently a random port is used that prevents the debug server from being used behind a firewall or in a container. > > > > > > > > New CSR [3] was created for this change and it needs to be reviewed as well. > > > > > > > > Man pages for jhsdb will be updated in a separate issue. > > > > > > > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > > > > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > > > > > > > > // delegate to the actual SA debug server. > > > > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > > > > > > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > > > > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > > > > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > > > > but I would prefer to address it in a separate issue. > > > > > > > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > > > > container and connecting to it with the GUI debugger. > > > > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > > > > > > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > > > > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > > > > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > > > > > > > Thank you, > > > > Daniil > > > > > > > > > > > > > > > > > > > > > > > > > >
