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