[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8037345 com/sun/jdi/* tests can fail, with race condition on log files
From:       Staffan Larsen <staffan.larsen () oracle ! com>
Date:       2014-03-28 20:58:03
Message-ID: B6252F4A-759A-4DF5-B3BD-204FF9349F40 () oracle ! com
[Download RAW message or body]

Dan, Serguei: Thanks!

On 28 mar 2014, at 19:23, serguei.spitsyn@oracle.com wrote:

> I'm Ok with fix as it is.
> 
> Thanks,
> Serguei
> 
> On 3/28/14 7:09 AM, Daniel D. Daugherty wrote:
> > 
> > On 3/28/14 8:02 AM, Staffan Larsen wrote:
> > > On 28 mar 2014, at 14:16, Daniel D. Daugherty <daniel.daugherty@oracle.com> \
> > > wrote: 
> > > > > http://cr.openjdk.java.net/~sla/8037345/webrev.00/
> > > > test/com/sun/jdi/ShellScaffold.sh
> > > > Wouldn't it be better to make grepForString() work for multiple
> > > > processes by making the temp file unique via '$$'? You'll have
> > > > to update the logic carefully since theFile can refer to either
> > > > a file you want to keep or the temp file.
> > > I thought about that, but wanted to avoid making the code more complex.
> > 
> > Your call, but I think fixing grepForString() is less complex than
> > the in-line logic that you had to use to get around this.
> > 
> > 
> > > (I also noticed that $$ had the same value for both calls to grepForString(). \
> > > $BASHPID on the other hand had different values (when running with bash, which \
> > > wasn’t the default shell). I’m not a good enough bash hacker to understand \
> > > why.)
> > 
> > That sounds very, very strange. The whole point of '$$' is to give you
> > a unique value for each shell process. If that's broken, then we have
> > much bigger problems.
> > 
> > In any case, I'm OK with what you have if you don't want to chase this
> > down any more.
> > 
> > Dan
> > 
> > 
> > > 
> > > /Staffan
> > > 
> > > 
> > > > Dan
> > > > 
> > > > 
> > > > On 3/28/14 5:56 AM, Staffan Larsen wrote:
> > > > > Please review this fix for the com/sun/jdi tests.
> > > > > 
> > > > > In grepForString(), the script sometimes creates a temporary file which is \
> > > > > used for grepping in. This file is a copy of the jdb outputfile and is \
> > > > > deleted at the end of grepForString(). If two processes execute \
> > > > > grepForString at the same time, there is a race where one process may \
> > > > > delete the temporary file that the other process is still using. \
> > > > > grepForString() is not written to be used bu multiple processes. Normally \
> > > > > grepForString is only called from the jdp process. 
> > > > > In waitForFinish() the main process is waiting for the jdb process to \
> > > > > finish. It does this in a loop checking if the pid still exists, and also \
> > > > > checking for some errors in the jdb outputfile. This last checking is the \
> > > > > problem since it uses jdbFailIfPresent (which calls grepForString( to do \
> > > > > this. Now grepForString is called from two different processes and we have \
> > > > > a race. This last usage was introduced by the fix for JDK-6946101. 
> > > > > The solution is to not call grepForString from the waitForFinish loop, but \
> > > > > instead revert to the old behavior of using grep directly. 
> > > > > webrev: http://cr.openjdk.java.net/~sla/8037345/webrev.00/
> > > > > bug: https://bugs.openjdk.java.net/browse/JDK-8037345
> > > > > 
> > > > > Thanks,
> > > > > /Staffan
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]