RFR: 8176717: GC log file handle leaked to child processes
leo.korinth at oracle.com
Tue Apr 3 16:24:38 UTC 2018
On 30/03/18 13:16, Thomas Stüfe wrote:
> Hi Leo,
> looks okay. I stated my reservations against adding this function and
> the platform specific code in its current form before, but I can
> certainly live with this fix. It follows what we did in os::open(), so
> it is consistent in that matter.
Thanks for (what I understand is) a review approval. I am still open for
a different solution if you have one you feel for. Below are some
alternatives that we have talked about (or I have thought about).
Dividing the fix to os_windows/os_posix will not get rid of the #if and
splitting it between aix/bsd/linux/solaris/windows will make the
solution much bigger, redundant and arguably harder to maintain.
I could possibly change it to have a variable or function in each
architecture that holds only the mode string for fopen (or NULL for no
support) and in that way change the "#if" to an "if" and remove the
LINUX_ONLY("e") BSD_ONLY("e") WINDOWS_ONLY("N"). I dislike it because it
spreads the solution to more files, but at least it reduces the use of
I could try to get the mode flag through autoconf, but that seems _very_
hard -- does success of fopen really mean that the mode is supported or
merely ignored? I believe it to be undefined by the c standard :-(, It
seems even harder to do an autoconf runtime test for the feature.
I could leave it to be fixed (only) by core-libs, but that I feel is
wrong for several reasons:
- We have a corresponding fix in os::open that says that we should
auto-close on exec from hotspot to be sure. And I agree on that.
- core-libs might not be able to fix it for legacy reasons
- it does not protect from when JNI creates new processes (I guess it is
I could make the change log-specific, but that makes little sense as if
I ought to do it in logging, I ought to do it on every fopen.
I was and still am hoping for more opinions to get a better solution or
at least a consensus on this solution. If I get none, I will continue
with the one I propose.
In the end, I will need a sponsor to eventually push the change.
> Thanks for fixing this!
> (btw, I am not sure which of my "2" Per meant, since I accidentally
> miscounted my points in the earlier mail.)
> On Thu, Mar 29, 2018 at 5:41 PM, Leo Korinth <leo.korinth at oracle.com
> <mailto:leo.korinth at oracle.com>> wrote:
> On 23/03/18 20:03, Leo Korinth wrote:
> Cross-posting this to both hotspot-dev and hotspot-runtime-dev
> to get more attention. Sorry.
> Original mail conversation can be found here:
> I need feedback to know how to continue.
> Below is a new webrev with Thomas' and Per's suggested name change
> from: os::fopen_retain(const char* path, const char* mode)
> to: os::fopen(const char* path, const char* mode)
> Full webrev:
> Review and/or comments please!
More information about the hotspot-runtime-dev