Review request 6985422: flush the output streams before OnError commands

[Chuck Rasbold]

On Sun, Sep 19, 2010 at 11:48 PM, David Holmes <David.Holmes at oracle.com>wrote:

> Chuck Rasbold said the following on 09/18/10 03:07:
>
>  On Thu, Sep 16, 2010 at 4:57 PM, David Holmes <David.Holmes at oracle.com<mailto:
>> David.Holmes at oracle.com>> wrote:
>>    Why was the ostream_abort() removed from the os::shutdown() path?
>>
>>    In the existing code we have:
>>
>>    VMError::report_and_die()
>>     -> os::abort()
>>       -> os::shutdown()
>>          -> ostream_abort()
>>
>>
>> It seems backward to me the the os-specific abort() calls ostream_abort().
>>
>> Ostream_init() and ostream_init_log() are called from create_vm(), I think
>> it would be preferable to clean-up the ostreams at a similar level.
>>
>
> I don't see how you can given that you may need the streams right up until
> you actually abort the process, and that is done down in the depths of the
> os::abort/shutdown code at the end of the error path. Normal VM termination
> is a different matter and ostream_exit() is used for that purpose.
>
>
>     and you've moved ostream_abort() into report_and_die(), but there
>>    are other exit paths that also call os::shutdown() and will no
>>    longer call ostream_abort():
>>
>>    vm_shutdown_during_initialization()
>>    -> vm_shutdown()
>>       -> os::shutdown()
>>
>>
>> Agreed that I missed vm_shutdown() in java.cpp.   I'd be happy to add a
>> call to ostream_abort() there.
>>
>
> I think this is the wrong place to "abort" the output streams. It only
> works, in my view, because the tty and gc streams don't actually get
> "aborted" just flushed. Otherwise I suspect that there might be use of those
> streams after the current call to ostream_abort().
>
>
>  Are there other call sites to os:shutdown() that I've missed?  If so,
>> perhaps it is safer to leave the originals calls in os::abort() as I've
>> underestimated the complexity of the shutdown code.
>>
>
> I don't think you missed anything else, but I think this is the wrong way
> to fix it.
>
>
>     Also with this move you need to fix this comment in ostream.cpp:
>>
>>    // ostream_abort() is called by os::abort() when VM is about to die.
>>    void ostream_abort() {
>>
>> True.
>>
>>    Why not just add explicit flush() calls where needed?
>>
>>
>> Flushing is pretty much what ostream_abort() does.  The compilation log
>> flushing code is short but non-trivial.  I think it is best not duplicate
>> the it.
>>
>
> I would rather see distinct flushing and "aborting" methods for the
> streams. ostream_abort() happens to also need to perform flushing, but
> flushing need not lead to aborting.
>

The "aborting" that ostream_abort() does is to flush and merge the
LogCompilation files.  That behavior is what we desire before OnError
commands are processed.

I could factor out an ostream_flush() function, but ithere's no place we'd
want to call it.

So is this OK?

http://cr.openjdk.java.net/~rasbold/6985422/webrev.00

-- Chuck


> David
>
>  -- Chuck
>>
>>
>>    David Holmes
>>
>>
>>        Contributed-by: rasbold at google.com <mailto:rasbold at google.com>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20100920/895c286a/attachment.html