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

[Robbin Ehn]

Thanks for fixing, looks good.

/Robbin

On 2018-05-03 12:01, Leo Korinth wrote:
> 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