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

Leo Korinth leo.korinth at oracle.com
Tue Mar 13 10:02:09 UTC 2018


> I agree with you on the proposed fix: to open the file - at least on 
> windows - with the inherit flag turned off. I still disagree with you 
> about the way this is done. I am not a bit fan on "one trick APIs" being 
> dropped into the os namespace for one singular purpose - I think we had 
> recently a similar discussion about an snprintf variant specific for 
> logging only.
> 
> Just counting are a couple of variants I would prefer:
> 
> 1) keep the API locally to logging, do not make it global. It is logging 
> specific, after all.
But is it really logging specific? I tried to do it as generic as 
possible. The _only_ behaviour I am forcing is that the descriptor 
should not leak (a property that arguably ought to be forced globally). 
I feel it is _more_ generic than os::open --- I am for example not 
forcing binary mode on anyone.
> 
> 2) Or even easier, just do this (logFileOutput.cpp):
> 
> const char* const LogFileOutput::FileOpenMode = WINDOWS_ONLY("aN") 
> NOT_WINDOWS("a");
> 
> that would fix windows. The other platforms do not have the problem if 
> spawning via Runtime.exec(), and the problem of 
> native-forking-and-handle-leaking is, while possible, rather theoretical.
> 
> 2) If you really want a new global API, rename the thing to just 
> "os::fopen()". Because after all you want to wrap a generic fopen() and 
> forbid handle inheritance, yes? This is the same thing the os::open() 
> sister function does too, so if you think you need that, give it a first 
> class name :) And we could use tests then too (I think we have gtests 
> for os::open()). In that case I also dislike the many ifdefs, so if you 
> keep the function in its current form, I'd prefer it fanned out for 
> different platforms, like os::open() does it.
Maybe the name os::fopen is better, I was afraid that it would give the 
wrongful impression that it was just a platform agnostic ::fopen. Just 
like os::open might not give the impression that it opens files in 
binary mode on windows.
> 
> Just my 5c, and tastes differ, so I'll wait what others say. I'll cc 
> Markus as the UL owner.

Thank you for the feedback! Lets see if the functionality is needed or 
wanted outside logging, if not I will remove it from "os" and just 
inline its use...

Thanks,
Leo
> Oh, I also think the bug desciption is a bit misleading, since this is 
> about the UL file handle in general, not only gc log. And mayit make 
> sense to post this in hotspot-runtime, not hotspot-dev?
> 
> Thanks and Best Regards, Thomas


More information about the hotspot-dev mailing list