RFR: 8176717: GC log file handle leaked to child processes

[Leo Korinth]

Hi Robbin,

On 02/05/18 14:39, Robbin Ehn wrote:
> Hi Leo,
> 
> I looked at http://cr.openjdk.java.net/~lkorinth/8176717/02/.
> 
> I think it would be good with some sort of error checking if fcntl 
> failed, maybe just an assert.
> 1282     if (fd != -1) {
> 1283       fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | FD_CLOEXEC);
> 1284     }
Changed to:
     if (fd != -1) {
       int fd_flags = fcntl(fd, F_GETFD);
       if (fd_flags != -1) {
         fcntl(fd, F_SETFD, fd_flags | FD_CLOEXEC);
       }
     }

I agree that this change is somewhat better and also looks more like the 
existing code that handles FD_CLOEXEC.

New full webrev:
http://cr.openjdk.java.net/~lkorinth/8176717/03

Testing:
TestInheritFD.java passed on my computer.
(running hs-tier{1,2} and TestInheritFD.java on mach5)

Thanks for the review!
/Leo

> Otherwise looks good, thanks, Robbin.
> 
> On 2018-03-12 14:20, Leo Korinth wrote:
>> Hi,
>>
>> This fix is for all operating systems though the problem only seams to 
>> appear on windows.
>>
>> I am creating a proxy function for fopen (os::fopen_retain) that 
>> appends the non-standard "e" mode for linux and bsds. For windows the 
>> "N" mode is used. For other operating systems, I assume that I can use 
>> fcntl F_SETFD FD_CLOEXEC. I think this will work for AIX, Solaris and 
>> other operating systems that do not support the "e" flag. Feedback 
>> otherwise please!
>>
>> The reason that I use the mode "e" and not only fcntl for linux and 
>> bsds is threefold. First, I still need to use mode flags on windows as 
>> it does not support fcntl. Second, I probably save a system call. 
>> Third, the change will be applied directly, and there will be no point 
>> in time (between system calls) when the process can leak the file 
>> descriptor, so it is safer.
>>
>> The test case forks three VMs in a row. By doing so we know that the 
>> second VM is opened with a specific log file. The third VM should have 
>> less open file descriptors (as it is does not use logging) which is 
>> checked using a UnixOperatingSystemMXBean. This is not possible on 
>> windows, so on windows I try to rename the file, which will not work 
>> if the file is opened (the actual reason the bug was opened).
>>
>> The added test case shows that the bug fix closes the log file on 
>> windows. The VM on other operating systems closed the log file even 
>> before the fix.
>>
>> Maybe the test case should be moved to a different path?
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8176717
>> https://bugs.openjdk.java.net/browse/JDK-8176809
>>
>> Webrev:
>> http://cr.openjdk.java.net/~lkorinth/8176717/00/
>>
>> Testing:
>> hs-tier1, hs-tier2 and TestInheritFD.java
>> (on 64-bit linux, solaris, windows and mac)
>>
>> Thanks,
>> Leo