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
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
