RE: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Langer, Christoph]

Hi Chris,

I guess it's the multitude of JVMs that get started one after each other and 
not in parallel.

So I'll push my fix then.

Best regards
Christoph

> -Original Message-
> From: Chris Plummer 
> Sent: Freitag, 13. September 2019 19:49
> To: Langer, Christoph ; Severin Gehwolf
> ; Thomas Stüfe 
> Cc: OpenJDK Serviceability 
> Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java
> fails intermittently
> 
> 3 minutes??? Sounds like something is wrong. What is the JVM doing
> during this time?
> 
> Chris
> 
> On 9/12/19 12:53 PM, Langer, Christoph wrote:
> > Hi Severin,
> >
> > that seems an interesting idea for an elegant solution. However, after
> trying this on a decently fast linux x86 box by leveraging one of these
> ProcessTools::startProcess methods that would wait for a certain output to
> appear in the child before returning, I figured that the elapsed runtime of 
> the
> test increases from about 3 seconds to 3 minutes. It evidently takes a lot
> longer to bootstrap a JVM and get the results of a first println than just
> forking a process and immediately accessing its proc filesystem. So I think we
> don't want to do that.
> >
> > I would like to go with this version (changed the comment to Thomas'
> suggestion): http://cr.openjdk.java.net/~clanger/webrevs/8230850.1/
> >
> > Chris, are you ok with it?
> >
> > Thanks
> > Christoph
> >
> >> -Original Message-
> >> From: Severin Gehwolf 
> >> Sent: Donnerstag, 12. September 2019 11:01
> >> To: Langer, Christoph ; Thomas Stüfe
> >> 
> >> Cc: OpenJDK Serviceability 
> >> Subject: Re: RFR (S): 8230850: Test
> sun/tools/jcmd/TestProcessHelper.java
> >> fails intermittently
> >>
> >> Hi Christoph,
> >>
> >> Have you considered to wait for TestProcess - the spawned processes - to
> >> print this on stdout:
> >>
> >> "The process started, pid: XXX"
> >>
> >> Once that's ready on stdout, checking the main class should always
> >> pass. I believe p.isAlive() check which is currrently done is
> >> insufficient.
> >>
> >> Thanks,
> >> Severin
> >>
> >> On Thu, 2019-09-12 at 08:12 +0000, Langer, Christoph wrote:
> >>> Hi Thomas,
> >>>
> >>> sounds reasonable, will do.
> >>>
> >>> Thanks
> >>> Christoph
> >>>
> >>> From: Thomas Stüfe 
> >>> Sent: Donnerstag, 12. September 2019 10:11
> >>> To: Langer, Christoph 
> >>> Cc: Chris Plummer ; OpenJDK Serviceability
> >> 
> >>> Subject: Re: RFR (S): 8230850: Test
> sun/tools/jcmd/TestProcessHelper.java
> >> fails intermittently
> >>> I'm fine with the patch if you would reshape the platform dependent
> >> comment. Proposal:
> >>> 
> >>> -// Depending on hw/os, process helper can return null here
> >>> -// because /proc//cmdline is not ready yet. To cover that
> case,
> >>>  // give it some retries.
> >>> ->
> >>> + getMainClass() may return NULL, e.g. due to timing issues. Attempt
> some
> >> limited retries.
> >>> 
> >>> I do not need another webrev.
> >>> Cheers, Thomas
> >>>
> >>>
> >>>
> >>>
> >>> On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph
> >>  wrote:
> >>>> Hi Chris, Thomas,
> >>>>
> >>>> thanks for looking at this. I was also wondering whether a fix in
> >> ProcessHelper would be appropriate. But I think introducing retries and
> >> delays in that code can do more harm than help.
> >>>> For this special test case, aiming to test the ProcessHelper 
> >>>> functionality
> >> (on Linux) only, the observed problem is that the /proc//cmdline file
> is
> >> not ready yet when it gets evaluated because the test can be quicker than
> >> the spawned processes. But in real life usage of jcmd this seems rather
> >> unlikely. One will probably use jcmd quite some time after a java process
> was
> >> started and /proc//cmdline should be ready. If then there are
> >> problems reading it, there are likely other issues which won’t go away by
> >> waiting. And for these cases the fallback is to use the attach framework,
> as
> >> implemented in ProcessArgumentMatcher, which provides some chance
> to
> >> be working still. And 

Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Chris Plummer]

3 minutes??? Sounds like something is wrong. What is the JVM doing 
during this time?


Chris

On 9/12/19 12:53 PM, Langer, Christoph wrote:

Hi Severin,

that seems an interesting idea for an elegant solution. However, after trying 
this on a decently fast linux x86 box by leveraging one of these 
ProcessTools::startProcess methods that would wait for a certain output to 
appear in the child before returning, I figured that the elapsed runtime of the 
test increases from about 3 seconds to 3 minutes. It evidently takes a lot 
longer to bootstrap a JVM and get the results of a first println than just 
forking a process and immediately accessing its proc filesystem. So I think we 
don't want to do that.

I would like to go with this version (changed the comment to Thomas' 
suggestion): http://cr.openjdk.java.net/~clanger/webrevs/8230850.1/

Chris, are you ok with it?

Thanks
Christoph


-Original Message-
From: Severin Gehwolf 
Sent: Donnerstag, 12. September 2019 11:01
To: Langer, Christoph ; Thomas Stüfe

Cc: OpenJDK Serviceability 
Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java
fails intermittently

Hi Christoph,

Have you considered to wait for TestProcess - the spawned processes - to
print this on stdout:

"The process started, pid: XXX"

Once that's ready on stdout, checking the main class should always
pass. I believe p.isAlive() check which is currrently done is
insufficient.

Thanks,
Severin

On Thu, 2019-09-12 at 08:12 +, Langer, Christoph wrote:

Hi Thomas,

sounds reasonable, will do.

Thanks
Christoph

From: Thomas Stüfe 
Sent: Donnerstag, 12. September 2019 10:11
To: Langer, Christoph 
Cc: Chris Plummer ; OpenJDK Serviceability



Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java

fails intermittently

I'm fine with the patch if you would reshape the platform dependent

comment. Proposal:


-// Depending on hw/os, process helper can return null here
-// because /proc//cmdline is not ready yet. To cover that case,
 // give it some retries.
->
+ getMainClass() may return NULL, e.g. due to timing issues. Attempt some

limited retries.


I do not need another webrev.
Cheers, Thomas




On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph

 wrote:

Hi Chris, Thomas,

thanks for looking at this. I was also wondering whether a fix in

ProcessHelper would be appropriate. But I think introducing retries and
delays in that code can do more harm than help.

For this special test case, aiming to test the ProcessHelper functionality

(on Linux) only, the observed problem is that the /proc//cmdline file is
not ready yet when it gets evaluated because the test can be quicker than
the spawned processes. But in real life usage of jcmd this seems rather
unlikely. One will probably use jcmd quite some time after a java process was
started and /proc//cmdline should be ready. If then there are
problems reading it, there are likely other issues which won’t go away by
waiting. And for these cases the fallback is to use the attach framework, as
implemented in ProcessArgumentMatcher, which provides some chance to
be working still. And this fallback should also cover the exotic case when jcmd
is issued too early.

After all, ProcessHelper::getMainClass also documents that its result can

be null.

@Thomas, as for your other points:
  PID reusage: Hm, maybe one can construct cases. However, I’d think the

/proc/pid files should be gone after a process ends. Or at least be
reconstructed if there were orphans and a new process reusing an old pid
gets started. But who knows what can happen – we’ll maybe see 

  Comment for Linux only issue: The test is in fact a Linux only test. See line

55: * @requires os.family == "linux". So, if we’ll eventually see
implementations for ProcessHelper::getMainClass on other platforms, this
comment might have to be adopted. But for the time being I guess it’s fine at
its current place.

Would you agree?

Best regards
Christoph

From: Chris Plummer 
Sent: Mittwoch, 11. September 2019 19:21
To: Thomas Stüfe ; Langer, Christoph



Cc: OpenJDK Serviceability 
Subject: Re: RFR (S): 8230850: Test

sun/tools/jcmd/TestProcessHelper.java fails intermittently

It does seem that the fix should be in ProcessHelper.java in

getMainClass(), or maybe even getCommandLine(). Fixing it in the test
implies that every user of getMainClass() should be doing something similar.
But then also note what ProcessArgumentMatch.check() is doing. It also
deals with getMainClass() returing null.

thanks,

Chris

On 9/11/19 6:59 AM, Thomas Stüfe wrote:

Hi Christoph,

in general I think this is fine. The increase-by-pow2 sleep time is odd

but okay :)

The whole things seems rather fragile and has a lot of question marks

but I think your fix does not make it worse. One fun error now is that with a
follow up java test reusing the PID we could get a wrong main class but I think
the chances are astronomi

Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Severin Gehwolf]

On Thu, 2019-09-12 at 19:53 +, Langer, Christoph wrote:
> Hi Severin,
> 
> that seems an interesting idea for an elegant solution. However,
> after trying this on a decently fast linux x86 box by leveraging one
> of these ProcessTools::startProcess methods that would wait for a
> certain output to appear in the child before returning, I figured
> that the elapsed runtime of the test increases from about 3 seconds
> to 3 minutes. It evidently takes a lot longer to bootstrap a JVM and
> get the results of a first println than just forking a process and
> immediately accessing its proc filesystem. So I think we don't want
> to do that.

Wow, that's unfortunate.

> I would like to go with this version (changed the comment to Thomas'
> suggestion): http://cr.openjdk.java.net/~clanger/webrevs/8230850.1/

Patch seems OK in this case.

Thanks,
Severin

> Chris, are you ok with it?
> 
> Thanks
> Christoph
> 
> > -Original Message-
> > From: Severin Gehwolf 
> > Sent: Donnerstag, 12. September 2019 11:01
> > To: Langer, Christoph ; Thomas Stüfe
> > 
> > Cc: OpenJDK Serviceability 
> > Subject: Re: RFR (S): 8230850: Test
> > sun/tools/jcmd/TestProcessHelper.java
> > fails intermittently
> > 
> > Hi Christoph,
> > 
> > Have you considered to wait for TestProcess - the spawned processes
> > - to
> > print this on stdout:
> > 
> > "The process started, pid: XXX"
> > 
> > Once that's ready on stdout, checking the main class should always
> > pass. I believe p.isAlive() check which is currrently done is
> > insufficient.
> > 
> > Thanks,
> > Severin
> > 
> > On Thu, 2019-09-12 at 08:12 +, Langer, Christoph wrote:
> > > Hi Thomas,
> > > 
> > > sounds reasonable, will do.
> > > 
> > > Thanks
> > > Christoph
> > > 
> > > From: Thomas Stüfe 
> > > Sent: Donnerstag, 12. September 2019 10:11
> > > To: Langer, Christoph 
> > > Cc: Chris Plummer ; OpenJDK
> > > Serviceability
> > 
> > > Subject: Re: RFR (S): 8230850: Test
> > > sun/tools/jcmd/TestProcessHelper.java
> > fails intermittently
> > > I'm fine with the patch if you would reshape the platform
> > > dependent
> > comment. Proposal:
> > > 
> > > -// Depending on hw/os, process helper can return null
> > > here
> > > -// because /proc//cmdline is not ready yet. To
> > > cover that case,
> > > // give it some retries.
> > > ->
> > > + getMainClass() may return NULL, e.g. due to timing issues.
> > > Attempt some
> > limited retries.
> > > 
> > > I do not need another webrev.
> > > Cheers, Thomas
> > > 
> > > 
> > > 
> > > 
> > > On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph
> >  wrote:
> > > > Hi Chris, Thomas,
> > > > 
> > > > thanks for looking at this. I was also wondering whether a fix
> > > > in
> > ProcessHelper would be appropriate. But I think introducing retries
> > and
> > delays in that code can do more harm than help.
> > > > For this special test case, aiming to test the ProcessHelper
> > > > functionality
> > (on Linux) only, the observed problem is that the
> > /proc//cmdline file is
> > not ready yet when it gets evaluated because the test can be
> > quicker than
> > the spawned processes. But in real life usage of jcmd this seems
> > rather
> > unlikely. One will probably use jcmd quite some time after a java
> > process was
> > started and /proc//cmdline should be ready. If then there are
> > problems reading it, there are likely other issues which won’t go
> > away by
> > waiting. And for these cases the fallback is to use the attach
> > framework, as
> > implemented in ProcessArgumentMatcher, which provides some chance
> > to
> > be working still. And this fallback should also cover the exotic
> > case when jcmd
> > is issued too early.
> > > > After all, ProcessHelper::getMainClass also documents that its
> > > > result can
> > be null.
> > > > @Thomas, as for your other points:
> > > >  PID reusage: Hm, maybe one can construct cases. However, I’d
> > > > think the
> > /proc/pid files should be gone after a process ends. Or at least be
> > reconstructed if there were orphans and a new process reusing an
> > old pid
> > gets started. But who knows what can happen – we’ll maybe s

RE: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Langer, Christoph]

Hi Severin,

that seems an interesting idea for an elegant solution. However, after trying 
this on a decently fast linux x86 box by leveraging one of these 
ProcessTools::startProcess methods that would wait for a certain output to 
appear in the child before returning, I figured that the elapsed runtime of the 
test increases from about 3 seconds to 3 minutes. It evidently takes a lot 
longer to bootstrap a JVM and get the results of a first println than just 
forking a process and immediately accessing its proc filesystem. So I think we 
don't want to do that.

I would like to go with this version (changed the comment to Thomas' 
suggestion): http://cr.openjdk.java.net/~clanger/webrevs/8230850.1/

Chris, are you ok with it?

Thanks
Christoph

> -Original Message-
> From: Severin Gehwolf 
> Sent: Donnerstag, 12. September 2019 11:01
> To: Langer, Christoph ; Thomas Stüfe
> 
> Cc: OpenJDK Serviceability 
> Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java
> fails intermittently
> 
> Hi Christoph,
> 
> Have you considered to wait for TestProcess - the spawned processes - to
> print this on stdout:
> 
> "The process started, pid: XXX"
> 
> Once that's ready on stdout, checking the main class should always
> pass. I believe p.isAlive() check which is currrently done is
> insufficient.
> 
> Thanks,
> Severin
> 
> On Thu, 2019-09-12 at 08:12 +, Langer, Christoph wrote:
> > Hi Thomas,
> >
> > sounds reasonable, will do.
> >
> > Thanks
> > Christoph
> >
> > From: Thomas Stüfe 
> > Sent: Donnerstag, 12. September 2019 10:11
> > To: Langer, Christoph 
> > Cc: Chris Plummer ; OpenJDK Serviceability
> 
> > Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java
> fails intermittently
> >
> > I'm fine with the patch if you would reshape the platform dependent
> comment. Proposal:
> >
> > 
> > -// Depending on hw/os, process helper can return null here
> > -// because /proc//cmdline is not ready yet. To cover that 
> > case,
> > // give it some retries.
> > ->
> > + getMainClass() may return NULL, e.g. due to timing issues. Attempt some
> limited retries.
> > 
> > I do not need another webrev.
> > Cheers, Thomas
> >
> >
> >
> >
> > On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph
>  wrote:
> > > Hi Chris, Thomas,
> > >
> > > thanks for looking at this. I was also wondering whether a fix in
> ProcessHelper would be appropriate. But I think introducing retries and
> delays in that code can do more harm than help.
> > >
> > > For this special test case, aiming to test the ProcessHelper functionality
> (on Linux) only, the observed problem is that the /proc//cmdline file is
> not ready yet when it gets evaluated because the test can be quicker than
> the spawned processes. But in real life usage of jcmd this seems rather
> unlikely. One will probably use jcmd quite some time after a java process was
> started and /proc//cmdline should be ready. If then there are
> problems reading it, there are likely other issues which won’t go away by
> waiting. And for these cases the fallback is to use the attach framework, as
> implemented in ProcessArgumentMatcher, which provides some chance to
> be working still. And this fallback should also cover the exotic case when 
> jcmd
> is issued too early.
> > >
> > > After all, ProcessHelper::getMainClass also documents that its result can
> be null.
> > >
> > > @Thomas, as for your other points:
> > >  PID reusage: Hm, maybe one can construct cases. However, I’d think the
> /proc/pid files should be gone after a process ends. Or at least be
> reconstructed if there were orphans and a new process reusing an old pid
> gets started. But who knows what can happen – we’ll maybe see 
> > >  Comment for Linux only issue: The test is in fact a Linux only test. See 
> > > line
> 55: * @requires os.family == "linux". So, if we’ll eventually see
> implementations for ProcessHelper::getMainClass on other platforms, this
> comment might have to be adopted. But for the time being I guess it’s fine at
> its current place.
> > >
> > > Would you agree?
> > >
> > > Best regards
> > > Christoph
> > >
> > > From: Chris Plummer 
> > > Sent: Mittwoch, 11. September 2019 19:21
> > > To: Thomas Stüfe ; Langer, Christoph
> 
> > > Cc: OpenJDK Serviceability 
> > > Subject: Re: RFR (S): 8230850: Test
> sun/tools/jcmd/TestProcessHelper.java fails

Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Severin Gehwolf]

Hi Christoph,

Have you considered to wait for TestProcess - the spawned processes - to print 
this on stdout:

"The process started, pid: XXX"

Once that's ready on stdout, checking the main class should always
pass. I believe p.isAlive() check which is currrently done is
insufficient.

Thanks,
Severin

On Thu, 2019-09-12 at 08:12 +, Langer, Christoph wrote:
> Hi Thomas,
>  
> sounds reasonable, will do.
>  
> Thanks
> Christoph
>  
> From: Thomas Stüfe  
> Sent: Donnerstag, 12. September 2019 10:11
> To: Langer, Christoph 
> Cc: Chris Plummer ; OpenJDK Serviceability 
> 
> Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java 
> fails intermittently
>  
> I'm fine with the patch if you would reshape the platform dependent comment. 
> Proposal:
>  
> 
> -// Depending on hw/os, process helper can return null here
> -// because /proc//cmdline is not ready yet. To cover that case,
> // give it some retries.
> -> 
> + getMainClass() may return NULL, e.g. due to timing issues. Attempt some 
> limited retries.
> 
> I do not need another webrev.
> Cheers, Thomas
>  
>  
>  
>  
> On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph  
> wrote:
> > Hi Chris, Thomas,
> >  
> > thanks for looking at this. I was also wondering whether a fix in 
> > ProcessHelper would be appropriate. But I think introducing retries and 
> > delays in that code can do more harm than help.
> >  
> > For this special test case, aiming to test the ProcessHelper functionality 
> > (on Linux) only, the observed problem is that the /proc//cmdline file 
> > is not ready yet when it gets evaluated because the test can be quicker 
> > than the spawned processes. But in real life usage of jcmd this seems 
> > rather unlikely. One will probably use jcmd quite some time after a java 
> > process was started and /proc//cmdline should be ready. If then there 
> > are problems reading it, there are likely other issues which won’t go away 
> > by waiting. And for these cases the fallback is to use the attach 
> > framework, as implemented in ProcessArgumentMatcher, which provides some 
> > chance to be working still. And this fallback should also cover the exotic 
> > case when jcmd is issued too early.
> >  
> > After all, ProcessHelper::getMainClass also documents that its result can 
> > be null.
> >  
> > @Thomas, as for your other points:
> >  PID reusage: Hm, maybe one can construct cases. However, I’d think the 
> > /proc/pid files should be gone after a process ends. Or at least be 
> > reconstructed if there were orphans and a new process reusing an old pid 
> > gets started. But who knows what can happen – we’ll maybe see 
> >  Comment for Linux only issue: The test is in fact a Linux only test. See 
> > line 55: * @requires os.family == "linux". So, if we’ll eventually see 
> > implementations for ProcessHelper::getMainClass on other platforms, this 
> > comment might have to be adopted. But for the time being I guess it’s fine 
> > at its current place.
> >  
> > Would you agree?
> >  
> > Best regards
> > Christoph
> >  
> > From: Chris Plummer  
> > Sent: Mittwoch, 11. September 2019 19:21
> > To: Thomas Stüfe ; Langer, Christoph 
> > 
> > Cc: OpenJDK Serviceability 
> > Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java 
> > fails intermittently
> >  
> > It does seem that the fix should be in ProcessHelper.java in 
> > getMainClass(), or maybe even getCommandLine(). Fixing it in the test 
> > implies that every user of getMainClass() should be doing something 
> > similar. But then also note what ProcessArgumentMatch.check() is doing. It 
> > also deals with getMainClass() returing null.
> > 
> > thanks,
> > 
> > Chris
> > 
> > On 9/11/19 6:59 AM, Thomas Stüfe wrote:
> > > Hi Christoph,
> > >  
> > > in general I think this is fine. The increase-by-pow2 sleep time is odd 
> > > but okay :)
> > >  
> > > The whole things seems rather fragile and has a lot of question marks but 
> > > I think your fix does not make it worse. One fun error now is that with a 
> > > follow up java test reusing the PID we could get a wrong main class but I 
> > > think the chances are astronomically low.
> > >  
> > > Only remark, you fix this in the platform shared code, if this is a Linux 
> > > only issue maybe it should be fixed in 
> > > /shared/projects/openjdk/jdk-jdk/source/src/jdk.jcmd/linux/classes/

RE: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Langer, Christoph]

Hi Thomas,

sounds reasonable, will do.

Thanks
Christoph

From: Thomas Stüfe 
Sent: Donnerstag, 12. September 2019 10:11
To: Langer, Christoph 
Cc: Chris Plummer ; OpenJDK Serviceability 

Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails 
intermittently

I'm fine with the patch if you would reshape the platform dependent comment. 
Proposal:



-// Depending on hw/os, process helper can return null here

-// because /proc//cmdline is not ready yet. To cover that case,

// give it some retries.

->

+ getMainClass() may return NULL, e.g. due to timing issues. Attempt some 
limited retries.



I do not need another webrev.

Cheers, Thomas







On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:
Hi Chris, Thomas,

thanks for looking at this. I was also wondering whether a fix in ProcessHelper 
would be appropriate. But I think introducing retries and delays in that code 
can do more harm than help.

For this special test case, aiming to test the ProcessHelper functionality (on 
Linux) only, the observed problem is that the /proc//cmdline file is not 
ready yet when it gets evaluated because the test can be quicker than the 
spawned processes. But in real life usage of jcmd this seems rather unlikely. 
One will probably use jcmd quite some time after a java process was started and 
/proc//cmdline should be ready. If then there are problems reading it, 
there are likely other issues which won’t go away by waiting. And for these 
cases the fallback is to use the attach framework, as implemented in 
ProcessArgumentMatcher, which provides some chance to be working still. And 
this fallback should also cover the exotic case when jcmd is issued too early.

After all, ProcessHelper::getMainClass also documents that its result can be 
null.

@Thomas, as for your other points:

  *   PID reusage: Hm, maybe one can construct cases. However, I’d think the 
/proc/pid files should be gone after a process ends. Or at least be 
reconstructed if there were orphans and a new process reusing an old pid gets 
started. But who knows what can happen – we’ll maybe see 
  *   Comment for Linux only issue: The test is in fact a Linux only test. See 
line 55: * @requires os.family == "linux". So, if we’ll eventually see 
implementations for ProcessHelper::getMainClass on other platforms, this 
comment might have to be adopted. But for the time being I guess it’s fine at 
its current place.

Would you agree?

Best regards
Christoph

From: Chris Plummer mailto:chris.plum...@oracle.com>>
Sent: Mittwoch, 11. September 2019 19:21
To: Thomas Stüfe mailto:thomas.stu...@gmail.com>>; 
Langer, Christoph mailto:christoph.lan...@sap.com>>
Cc: OpenJDK Serviceability 
mailto:serviceability-dev@openjdk.java.net>>
Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails 
intermittently

It does seem that the fix should be in ProcessHelper.java in getMainClass(), or 
maybe even getCommandLine(). Fixing it in the test implies that every user of 
getMainClass() should be doing something similar. But then also note what 
ProcessArgumentMatch.check() is doing. It also deals with getMainClass() 
returing null.

thanks,

Chris

On 9/11/19 6:59 AM, Thomas Stüfe wrote:
Hi Christoph,

in general I think this is fine. The increase-by-pow2 sleep time is odd but 
okay :)

The whole things seems rather fragile and has a lot of question marks but I 
think your fix does not make it worse. One fun error now is that with a follow 
up java test reusing the PID we could get a wrong main class but I think the 
chances are astronomically low.

Only remark, you fix this in the platform shared code, if this is a Linux only 
issue maybe it should be fixed in 
/shared/projects/openjdk/jdk-jdk/source/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java
 instead? If not, I would remove at least the /proc//cmdline comment since 
this is quite platform specific.

Cheers, Thomas


On Wed, Sep 11, 2019 at 2:39 PM Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:
Hi,

please review this change for test sun/tools/jcmd/TestProcessHelper.java to 
make it more robust.

Bug: https://bugs.openjdk.java.net/browse/JDK-8230850
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8230850.0/

This Linux only test is starting several Java processes and then tries to 
figure out the main class by invoking jdk.jcmd's linux specific ProcessHelper 
implementation which parses the contents of /proc//cmdline.
Under some circumstances, the test already attempts to read /proc//cmdline 
before it actually exists or is filled with data. This can be fixed with some 
sleeps/retries to wait for that data to be ready.
In the actual jcmd tool, such behavior of ProcessHelper. getMainClass should 
not be an issue because it is handled in ProcessArgumentMatcher [0].

Thanks
Christoph

[0] 
http://hg.openjdk.java.net/jdk/jdk

Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Thomas Stüfe]

I'm fine with the patch if you would reshape the platform dependent
comment. Proposal:



-// Depending on hw/os, process helper can return null here-
 // because /proc//cmdline is not ready yet. To cover that
case,// give it some retries.

->

+ getMainClass() may return NULL, e.g. due to timing issues. Attempt
some limited retries.



I do not need another webrev.

Cheers, Thomas





On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph 
wrote:

> Hi Chris, Thomas,
>
>
>
> thanks for looking at this. I was also wondering whether a fix in
> ProcessHelper would be appropriate. But I think introducing retries and
> delays in that code can do more harm than help.
>
>
>
> For this special test case, aiming to test the ProcessHelper functionality
> (on Linux) only, the observed problem is that the /proc//cmdline file
> is not ready yet when it gets evaluated because the test can be quicker
> than the spawned processes. But in real life usage of jcmd this seems
> rather unlikely. One will probably use jcmd quite some time after a java
> process was started and /proc//cmdline should be ready. If then there
> are problems reading it, there are likely other issues which won’t go away
> by waiting. And for these cases the fallback is to use the attach
> framework, as implemented in ProcessArgumentMatcher, which provides some
> chance to be working still. And this fallback should also cover the exotic
> case when jcmd is issued too early.
>
>
>
> After all, ProcessHelper::getMainClass also documents that its result can
> be null.
>
>
>
> @Thomas, as for your other points:
>
>- PID reusage: Hm, maybe one can construct cases. However, I’d think
>the /proc/pid files should be gone after a process ends. Or at least be
>reconstructed if there were orphans and a new process reusing an old pid
>gets started. But who knows what can happen – we’ll maybe see 
>- Comment for Linux only issue: The test is in fact a Linux only test.
>See line 55: * @requires os.family == "*linux*". So, if we’ll
>eventually see implementations for ProcessHelper::getMainClass on other
>platforms, this comment might have to be adopted. But for the time being I
>guess it’s fine at its current place.
>
>
>
> Would you agree?
>
>
>
> Best regards
>
> Christoph
>
>
>
> *From:* Chris Plummer 
> *Sent:* Mittwoch, 11. September 2019 19:21
> *To:* Thomas Stüfe ; Langer, Christoph <
> christoph.lan...@sap.com>
> *Cc:* OpenJDK Serviceability 
> *Subject:* Re: RFR (S): 8230850: Test
> sun/tools/jcmd/TestProcessHelper.java fails intermittently
>
>
>
> It does seem that the fix should be in ProcessHelper.java in
> getMainClass(), or maybe even getCommandLine(). Fixing it in the test
> implies that every user of getMainClass() should be doing something
> similar. But then also note what ProcessArgumentMatch.check() is doing. It
> also deals with getMainClass() returing null.
>
> thanks,
>
> Chris
>
> On 9/11/19 6:59 AM, Thomas Stüfe wrote:
>
> Hi Christoph,
>
>
>
> in general I think this is fine. The increase-by-pow2 sleep time is odd
> but okay :)
>
>
>
> The whole things seems rather fragile and has a lot of question marks but
> I think your fix does not make it worse. One fun error now is that with a
> follow up java test reusing the PID we could get a wrong main class but I
> think the chances are astronomically low.
>
>
>
> Only remark, you fix this in the platform shared code, if this is a Linux
> only issue maybe it should be fixed
> in 
> /shared/projects/openjdk/jdk-jdk/source/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java
> instead? If not, I would remove at least the /proc//cmdline comment
> since this is quite platform specific.
>
>
>
> Cheers, Thomas
>
>
>
>
>
> On Wed, Sep 11, 2019 at 2:39 PM Langer, Christoph <
> christoph.lan...@sap.com> wrote:
>
> Hi,
>
>
>
> please review this change for test sun/tools/jcmd/TestProcessHelper.java
> to make it more robust.
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8230850
>
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8230850.0/
>
>
>
> This Linux only test is starting several Java processes and then tries to
> figure out the main class by invoking jdk.jcmd's linux specific
> ProcessHelper implementation which parses the contents of
> /proc//cmdline.
>
> Under some circumstances, the test already attempts to read
> /proc//cmdline before it actually exists or is filled with data. This
> can be fixed with some sleeps/retries to wait for that data to be ready.
>
> In the actual jcmd tool, such behavior of ProcessHelper. getMainClass
> should not be an issue because it is handled in ProcessArgumentMatcher [0].
>
>
>
> Thanks
>
> Christoph
>
>
>
> [0]
> http://hg.openjdk.java.net/jdk/jdk/file/8b08eaf9a0eb/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java#l86
>
>
>
>
>

RE: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Langer, Christoph]

Hi Chris, Thomas,

thanks for looking at this. I was also wondering whether a fix in ProcessHelper 
would be appropriate. But I think introducing retries and delays in that code 
can do more harm than help.

For this special test case, aiming to test the ProcessHelper functionality (on 
Linux) only, the observed problem is that the /proc//cmdline file is not 
ready yet when it gets evaluated because the test can be quicker than the 
spawned processes. But in real life usage of jcmd this seems rather unlikely. 
One will probably use jcmd quite some time after a java process was started and 
/proc//cmdline should be ready. If then there are problems reading it, 
there are likely other issues which won’t go away by waiting. And for these 
cases the fallback is to use the attach framework, as implemented in 
ProcessArgumentMatcher, which provides some chance to be working still. And 
this fallback should also cover the exotic case when jcmd is issued too early.

After all, ProcessHelper::getMainClass also documents that its result can be 
null.

@Thomas, as for your other points:

  *   PID reusage: Hm, maybe one can construct cases. However, I’d think the 
/proc/pid files should be gone after a process ends. Or at least be 
reconstructed if there were orphans and a new process reusing an old pid gets 
started. But who knows what can happen – we’ll maybe see 
  *   Comment for Linux only issue: The test is in fact a Linux only test. See 
line 55: * @requires os.family == "linux". So, if we’ll eventually see 
implementations for ProcessHelper::getMainClass on other platforms, this 
comment might have to be adopted. But for the time being I guess it’s fine at 
its current place.

Would you agree?

Best regards
Christoph

From: Chris Plummer 
Sent: Mittwoch, 11. September 2019 19:21
To: Thomas Stüfe ; Langer, Christoph 

Cc: OpenJDK Serviceability 
Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails 
intermittently

It does seem that the fix should be in ProcessHelper.java in getMainClass(), or 
maybe even getCommandLine(). Fixing it in the test implies that every user of 
getMainClass() should be doing something similar. But then also note what 
ProcessArgumentMatch.check() is doing. It also deals with getMainClass() 
returing null.

thanks,

Chris

On 9/11/19 6:59 AM, Thomas Stüfe wrote:
Hi Christoph,

in general I think this is fine. The increase-by-pow2 sleep time is odd but 
okay :)

The whole things seems rather fragile and has a lot of question marks but I 
think your fix does not make it worse. One fun error now is that with a follow 
up java test reusing the PID we could get a wrong main class but I think the 
chances are astronomically low.

Only remark, you fix this in the platform shared code, if this is a Linux only 
issue maybe it should be fixed in 
/shared/projects/openjdk/jdk-jdk/source/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java
 instead? If not, I would remove at least the /proc//cmdline comment since 
this is quite platform specific.

Cheers, Thomas


On Wed, Sep 11, 2019 at 2:39 PM Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:
Hi,

please review this change for test sun/tools/jcmd/TestProcessHelper.java to 
make it more robust.

Bug: https://bugs.openjdk.java.net/browse/JDK-8230850
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8230850.0/

This Linux only test is starting several Java processes and then tries to 
figure out the main class by invoking jdk.jcmd's linux specific ProcessHelper 
implementation which parses the contents of /proc//cmdline.
Under some circumstances, the test already attempts to read /proc//cmdline 
before it actually exists or is filled with data. This can be fixed with some 
sleeps/retries to wait for that data to be ready.
In the actual jcmd tool, such behavior of ProcessHelper. getMainClass should 
not be an issue because it is handled in ProcessArgumentMatcher [0].

Thanks
Christoph

[0] 
http://hg.openjdk.java.net/jdk/jdk/file/8b08eaf9a0eb/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java#l86

Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Chris Plummer]

  
  
It does seem that the fix should be in
  ProcessHelper.java in getMainClass(), or maybe even
  getCommandLine(). Fixing it in the test implies that every user of
  getMainClass() should be doing something similar. But then also
  note what ProcessArgumentMatch.check() is doing. It also deals
  with getMainClass() returing null.
  
  thanks,
  
  Chris
  
  On 9/11/19 6:59 AM, Thomas Stüfe wrote:


  
  Hi Christoph,


in general I think this is fine. The increase-by-pow2 sleep
  time is odd but okay :)


The whole things seems rather fragile and has a lot of
  question marks but I think your fix does not make it worse.
  One fun error now is that with a follow up java test reusing
  the PID we could get a wrong main class but I think the
  chances are astronomically low.


Only remark, you fix this in the platform shared code, if
  this is a Linux only issue maybe it should be fixed
in /shared/projects/openjdk/jdk-jdk/source/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java
  instead? If not, I would remove at least the
  /proc//cmdline comment since this is quite platform
  specific.


Cheers, Thomas


  
  
  
On Wed, Sep 11, 2019 at 2:39
  PM Langer, Christoph 
  wrote:


  

  Hi,
   
  please review this
  change for test sun/tools/jcmd/TestProcessHelper.java
  to make it more robust.
   
  Bug: 
https://bugs.openjdk.java.net/browse/JDK-8230850

  Webrev: 
http://cr.openjdk.java.net/~clanger/webrevs/8230850.0/
   
  This Linux only
  test is starting several Java processes and then tries
  to figure out the main class by invoking jdk.jcmd's
  linux specific ProcessHelper implementation which
  parses the contents of /proc//cmdline.
  Under some
  circumstances, the test already attempts to read
  /proc//cmdline before it actually exists or
  is filled with data. This can be fixed with some
  sleeps/retries to wait for that data to be ready.
  In the actual jcmd
  tool, such behavior of ProcessHelper. getMainClass
  should not be an issue because it is handled in
  ProcessArgumentMatcher [0].
   
  Thanks
  Christoph
   
  [0] 
http://hg.openjdk.java.net/jdk/jdk/file/8b08eaf9a0eb/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java#l86
   

  

  


  

Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

[Thomas Stüfe]

Hi Christoph,

in general I think this is fine. The increase-by-pow2 sleep time is odd but
okay :)

The whole things seems rather fragile and has a lot of question marks but I
think your fix does not make it worse. One fun error now is that with a
follow up java test reusing the PID we could get a wrong main class but I
think the chances are astronomically low.

Only remark, you fix this in the platform shared code, if this is a Linux
only issue maybe it should be fixed
in 
/shared/projects/openjdk/jdk-jdk/source/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java
instead? If not, I would remove at least the /proc//cmdline comment
since this is quite platform specific.

Cheers, Thomas


On Wed, Sep 11, 2019 at 2:39 PM Langer, Christoph 
wrote:

> Hi,
>
>
>
> please review this change for test sun/tools/jcmd/TestProcessHelper.java
> to make it more robust.
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8230850
>
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8230850.0/
>
>
>
> This Linux only test is starting several Java processes and then tries to
> figure out the main class by invoking jdk.jcmd's linux specific
> ProcessHelper implementation which parses the contents of
> /proc//cmdline.
>
> Under some circumstances, the test already attempts to read
> /proc//cmdline before it actually exists or is filled with data. This
> can be fixed with some sleeps/retries to wait for that data to be ready.
>
> In the actual jcmd tool, such behavior of ProcessHelper. getMainClass
> should not be an issue because it is handled in ProcessArgumentMatcher [0].
>
>
>
> Thanks
>
> Christoph
>
>
>
> [0]
> http://hg.openjdk.java.net/jdk/jdk/file/8b08eaf9a0eb/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java#l86
>
>
>