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

Tue Apr 3 18:30:44 UTC 2018

Hi Leo,

On Tue, Apr 3, 2018 at 6:24 PM, Leo Korinth <leo.korinth at oracle.com> wrote:

> Hi Thomas,
>
> 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.

It was a fully approved review from my point. As I stated, I can live with
this fix. I am fine with you shipping this fix in its current form.

You cannot make everyone completely happy, nor is that even necessary. In
my eyes, the review process shall ensure that nothing gets submitted which
goes strongly against the grain or taste of reviewers or against the
community (e.g. like systemd). However, that process is not precise; there
will always be gray areas and many matters of taste.

Part of being reviewer is to know when to reign in ones own preferences for
the benefit of having a solution and not torturing the patch submitter more
than necessary. This is what I tried to convey here: it is not my solution
no. 1, but among the space of possible solutions it is one I can sure live
with.

So, whatever else I write below are just idle musings. In the end, I am
fine with this fix in its current form. Ship it.

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.
>
>
Yet this is what is usually done in hs coding - see e.g. in the case of
os::open(). The question of how many platform ifdefs are one-too-many and
at which point one starts to blossom out into platform specific files is a
matter of taste, and different reviewers have different pain barriers. I
think I am somewhere in the middle.

Also, I usually do "as the romans do" with regards to code. So, if I see
that the original code maintainers show strong preference for one side or
the other, I lean into the same side taste wise.

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 the
> preprocessor.
>
>
Dislike it too.

> 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.
>
>
Hard to maintain, yes, and also unnecessary.

> 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
> allowed)
>
> 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.
>
>
Oh right, you are not committer.

I am currently in vacation, so if I am to push this you will have to wait
2-3 weeks. Also, I am outside Oracle - while we outsiders can sponsor and
push now too, it may still go faster if you ask someone from Oracle to
sponsor this.

Kind Regards, and thanks for your work,

Thomas

> /Leo
>
> Thanks for fixing this!
>>
>> Thomas
>>
>> (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:
>>
>>         Hi!
>>
>>         Cross-posting this to both hotspot-dev and hotspot-runtime-dev
>>         to get more attention. Sorry.
>>
>>         Original mail conversation can be found here:
>>         http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-Marc
>> h/030637.html
>>         <http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-Mar
>> ch/030637.html>
>>
>>         I need feedback to know how to continue.
>>
>>         Thanks,
>>         Leo
>>
>>
>>     Hi!
>>
>>     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:
>>     http://cr.openjdk.java.net/~lkorinth/8176717/02/
>>     <http://cr.openjdk.java.net/~lkorinth/8176717/02/>
>>
>>     Review and/or comments please!
>>
>>     Thanks,
>>     Leo
>>
>>
>>