JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

Fri Nov 20 17:41:07 UTC 2015

Right.  I know attachments get trashed sometimes so hopefully this hacked up code in-lines correctly:

=====================================
private Exception formatThrown;
    private volatile Handler lastHandler;

    private void reportError(Handler h, Exception ex) {
        if (h == null) {
           h = this.lastHandler; 
        }

        if (h != null) {
            this.lastHandler = h;
            if (ex == null) {
               ex = set(null); 
            }

            if (ex != null) {
               h.reportError(null, ex, ErrorManager.FORMAT_FAILURE);
            }
        } else {
            if (ex != null) {
                set(ex);
            }
        }
    }

    private synchronized Exception set(Exception e) {
        Exception l = this.formatThrown;
        this.formatThrown = e;
        return l;
    }
==========================================

Then in the formatter.getHead/getTail call reportError(h, null) and in formatMessage call reportError(null, ex).  This example can be improved by clearing the handler/exception in getTail and avoid some redundant volatile stores but you get the idea.

Jason

________________________________________
From: Daniel Fuchs <daniel.fuchs at oracle.com>
Sent: Friday, November 20, 2015 11:04 AM
To: Jason Mehrens; Alexander Fomin; core-libs-dev at openjdk.java.net; mandy.chung at oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

On 20/11/15 17:55, Jason Mehrens wrote:
> Hi Daniel,
>
> Well I'm sure the authors of the unit tests wrote code that never leaks the handlers they have created right? :)
>
> If urgency or frequency of the reporting is required then capture the handler in getHead as formatter state.  The write code to report the exception under all possible states:
> 1. if exception present and getHead is called then report it to non null Handler and clear (JDK-6351685).

oh - I see. I hadn't thought of that. That's actually a very good
suggestion. So the Formatter could access the Handler's ErrorManager
and if that's not null it could call  handler.errorManager.error(...)

That's what you are suggesting - right?

best regards,

-- daniel

> 2. if exception happens during format and we have a handler captured from getHead then report otherwise store it.
> 3. if exception is present during getTail then report it to a non null Handler and clear the exception and hander.
>
> That means you'll have to add super.getHead and super.getTail calls in XMLFormatter too.
>
> I'm really leaning towards removing this try/catch (https://blogs.oracle.com/darcy/entry/kinds_of_compatibility).  Handlers already expect that Formatter.format->formattMessage will fail.  After all if they didn't fail we wouldn't have specified ErrorManager.FORMAT_FAILURE.  Take a look at StreamHandler.publish.  Or the handler implementation goes in the opposite direction and makes the publish method exception hostile which is already a violation of the spec.
>
> It pains me to say it but, as long as you don't break the SLF4J bridge handler then you have covered most of the JUL users.
>
> Jason
>
>
> ________________________________________
> From: Daniel Fuchs <daniel.fuchs at oracle.com>
> Sent: Friday, November 20, 2015 9:32 AM
> To: Jason Mehrens; Alexander Fomin; core-libs-dev at openjdk.java.net; mandy.chung at oracle.com
> Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
>
> On 20/11/15 15:47, Jason Mehrens wrote:
>> Alexander,
>>
>> Why not just cache the last exception in the formatter and use getTail to clear it and report it?  Since formatter is in the same package as Handler you will have elevated access to the error manager through Handler.reportError.  That also makes it so you don't have to change the public API of Formatter.
>
> Hi Jason,
>
> That would mean that you won't see the exception until
> the handler is closed. Not sure whether that matters much.
> ErrorManager looks already bizarre to me. But at least
> with ErrorManager it looks as if someone who cares could
> set his/her own ErrorManager on the formatter (with hopefully
> a more sensible implementation).
>
> I have no specific opinion on the subject I'm in favor
> of taking the solution that is the least likely to cause
> compatibility issues :-)
>
> best regards,
>
> -- daniel
>
>>
>> Jason
>>
>> ________________________________________
>> From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> on behalf of Alexander Fomin <alexander.fomin at oracle.com>
>> Sent: Friday, November 20, 2015 7:48 AM
>> To: core-libs-dev at openjdk.java.net; Daniel Fuchs; mandy.chung at oracle.com
>> Subject: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage()       swallows Exceptions
>>
>> Hi,
>> please, review this patch to report errors  in
>> java.util.logging.Formatter#formatMessage().
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8137005
>> Webrev: http://cr.openjdk.java.net/~dfuchs/alexander/8137005/webrev.00
>>
>> Summary:
>>        j.u.logging.Formatter#formatMessage() swallows exceptions that
>> happening during formatting of a message. In the result the exceptions
>> are lost and users don't know about reasons why the message hasn't been
>> formatted as expected. We would avoid to throw any exceptions in
>> Formatter#formatMessage() from compatibility stand point. To report an
>> error  in consistent way we have to pass ErrorManager in Formatter. It's
>> require API changes. So, I'm going to file CCC when if the fix approved.
>>        The suggested fix is to add 2 new methods in j.u.logging.Formatter
>> to set/get an ErrorManager, update Formatter#formatMessage() to report
>> errors via the ErrorManager and update Handler to pass errorManager to
>> Formatter.
>>
>> Testing:
>>        A couple of new regression tests have been created:
>>            test/java/util/logging/Test8137005.java - real case provided by
>> users
>>            test/java/util/logging/NullErrorManagerTest.java - additional
>> check to make sure no NPE showed if ErrorManager isn't set. Beside of
>> this touched new API methods.
>>
>>        Logging regression tests have been run:
>>            jdk/test/java/util/logging
>>            jdk/test/closed/java/util/logging
>>            jdk/test/sun/util/logging
>>        All tests passed passed.
>>
>> JPRT:
>> http://sthjprt.uk.oracle.com/archives/2015/11/2015-11-19-143523.gtee.dev/
>> failures in the job are known issues and not related to the fix.
>>
>> Thanks,
>> Alexander
>>
>>
>>
>