RFR: JDK-8232834: RunTest sometimes fails to produce valid exitcode.txt

[Erik Joelsson]

On 2019-10-23 00:42, Magnus Ihse Bursie wrote:
> On 2019-10-22 23:07, Erik Joelsson wrote:
>> When RunTest.gmk runs jtreg tests, it prints the exitcode of jtreg 
>> into a file named exitcode.txt. Soemtimes, this fails and the 
>> exitcode.txt file is left empty. This is causing trouble in automated 
>> testing where the surrounding framework is expecting to check the 
>> result in that file.
>>
>> I believe this is caused by a race in bash. We have seen similar 
>> races before and there is even a comment about it in the 
>> documentation for the ExecuteWithLog macro. If there is redirect of 
>> stdout in a command executed by ExecuteWithLog, that command must be 
>> surrounded by parens to run in a subshell. I first discovered this 
>> solution in JDK-8158629.
> Yikes! It's not the first time, yes. Do you think it would make sense 
> to put in the extra parenthesis in the ExecuteWithLog itself? I guess 
> the main problem with this is that we incur an extra shell invocation 
> for each call, even if it's not needed. Normally this would not be 
> even noticeable, but on Windows it might..? Otoh, being race free is 
> more important, and leaving the responsibility for this to the caller 
> of ExecuteWithLog is just too easy to get wrong, as we have seen 
> multiple times.
Yes, I've been torn about this. On Windows I made measurable build speed 
improvements some years ago by minimizing the extra shell invocations 
for compile commands, so I didn't like the idea of introducing an 
unconditional extra shell. Another option could be to scan the input for 
'[<>]' in ExecuteWithLog and only add it if found. It will certainly 
make the ExecuteWithLog body convoluted but may be a better solution.
>>
>> This patch puts parens around the jtreg and other test framework 
>> calls in RunTest.gmk.
>>
>> Webrev: http://cr.openjdk.java.net/~erikj/8232834/webrev.jdk14.01/
> Looks good to me. Even if we decide to change ExecuteWithLog itself 
> later on, I think it makes sense to fix this here and now.
>
Thanks!

/Erik