RFR: 8176717: GC log file handle leaked to child processes (original) (raw)

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 = WINDOWSONLY("aN") NOTWINDOWS("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