Hi Alex,
Thank you for reviewing this change.
Best regards,
Daniil
On 3/17/20, 11:58 AM, "Alex Menkov" <alexey.men...@oracle.com> wrote:
LGTM
--alex
On 03/17/2020 11:40, Daniil Titov wrote:
> Hi Alex,
>
> Please review a new version of the fix that removes the old version of
the code that tried to handle the "port in use" case.
>
> Testing: Mach5 tests for sun/tools/jstatd/ successfully passed 100
times. Tier1-tier3 tests successfully passed.
>
> [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.02
> [2] https://bugs.openjdk.java.net/browse/JDK-8240711
>
> Thanks,
> Daniil
>
>
>
> On 3/16/20, 5:38 PM, "Daniil Titov" <daniil.x.ti...@oracle.com> wrote:
>
> Hi Alex,
>
> Yes, I did test the change by modifying the test to use the RMI
port that is already in use
> ( the stack trace in the original email was exact from this changed
test) and then ensured that with the fix
> the such issue is properly handled.
>
> I will send a new version of the webrev that removes the old version
of the code that tried to handle the "port in use" case.
>
> Thanks!
>
> Best regards,
> Daniil
>
>
>
>
> On 3/16/20, 4:47 PM, "Alex Menkov" <alexey.men...@oracle.com> wrote:
>
> I don't agree.
> The code handles exact the same "port in use" case for the same
tool.
> So it either works or doesn't.
> And have 2 code blocks which suppose to do the same makes the
code messy.
> BTW did you tested the change (I mean craft the test to get
"port in
> use" error)?
>
> --alex
>
> On 03/16/2020 16:17, Daniil Titov wrote:
> > Resending with the corrected subject ...
> >
> > Hi Alex,
> >
> > Yes, you are right, class JstatdTest has the code that is
supposed to handle the "port in use"
> > case but at least for this specific test
(sun/tools/jstatd/TestJstatdPort.java) it doesn't work.
> >
> > Since there are multiple tests in sun/tools/jstatd/* folder
that use this class and different ports
> > might be subject to the "port in use" error and taking into
account that it's hard to reproduce such case
> > I found it safer to leave the original code and just augment
it with what was missing for this specific
> > case rather than completely replacing it.
> >
> > Best regards,
> > Daniil
> >
> > On 3/16/20, 4:02 PM, "Alex Menkov" <alexey.men...@oracle.com>
wrote:
> >
> > Hi Daniil,
> >
> > Looks like the test is supposed to handle "port in use"
issue (see lines
> > 103-114).
> > I suppose in case "port in use" jstatd exits, but
> > ProcessTools.startProcess() continue to wait for "jstatd
started" message.
> >
> > --alex
> >
> > On 03/16/2020 12:00, Daniil Titov wrote:
> > > Please review the change [1] that fixes the
intermittent failure of the test.
> > >
> > > The problem here is that if the RMI port is in use than
the test keep waiting for "jstatd started (bound to " to appear in the process
output and in this case
> > > It doesn't happen.
> > >
> > > at
java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232)
> > > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205)
> > > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133)
> > > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254)
> > > at
jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153)
> > > at jdk.test.lib.thread.XRun.run(XRun.java:40)
> > > at
java.lang.Thread.run(java.base@15-internal/Thread.java:832)
> > > at
jdk.test.lib.thread.TestThread.run(TestThread.java:123)
> > >
> > > Testing: Mach5 tests for sun/tools/jstatd/ successfully
passed. Tier1-tier3 tests are still in progress.
> > >
> > > [1]
http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/
> > > [2] https://bugs.openjdk.java.net/browse/JDK-8240711
> > >
> > >
> > > Thank you,
> > > Daniil
> > >
> > >
> > >
> >
> >
> >
>
>
>
>
