JDK 9 RFR : java.util.logging.Formatter#formatMessage() swallows Exceptions
alexander.fomin at oracle.com
Mon Nov 23 15:27:31 UTC 2015
On 20.11.2015 22:15, Jason Mehrens wrote:
> I see your point. It is also out of spec for Handler.setFormatter to actually call Formatter.setErrorManager. What if there are subclasses of formatter that exist today with a setErrorManager method? Also not all handlers have one formatter or actually call super.setFormatter since you can't unseal the super handler (GAE).
Good point. You are right, modifying Handler.setErrorManager and
Handler.setFormatter methods by the way to call
Formatter.setErrorManger() we may introduce undesirable behavior. For
example, if Handler is configured with a custom Formatter with a custom
ErrorManger specified, we may spoil the ErrorManager in that Formatter.
Probably the better solution would be to specify the default
ErrorManager for a Formatter in the constructor of specific Handler. But
seems we can't just call getErrorManager() in ConsoleHandler
constructor, for example, as far as the subclass isn't fully initialized
> I still lean toward letting the exceptions fly since there is already an expectation that formatters fail. Is the concern that the handler will fail or that the program will fail?
Well, actually my initial thought was to let the exceptions to be thrown
from Formatter to corresponding logging Handler where it should be
processed and reported to a registered ErrorManager. But we've given up
from this idea because:
- would require a CCC any way.
- would probably require a release note since it will alter the existing
- the current implementation of ErrorManager will cause a
printStackTrace() - but only the first time such an error happens in
Handler. So that, the errors in Formatter will be swallowed any way if
some error happened before.
- this approach may cause an NPE if the concrete Handler subclass has
not installed any ErrorManager and does not override reportError. So,
there's the issue of potentially breaking existing code.
So, I was looking for a solution that shouldn't change any existing
behavior, shouldn't break a code, should work in all cases, should allow
to a Formatter report about its internal errors itself without relay on
the corresponding Handler where the Exception could be intentionally
> I've recently been tinkering with a custom filter that internally uses a formatter and have prototyped adding get/setErrorManager method to it and it just becomes a mess because to have start thinking about more corner cases and adding more to the spec.
What is wrong with get/serErrorManager for Formatter? It should be just
a way for Formatter to report its internal errors.
> I really think that if you are going to add the 3 ErrorManager methods you should only add them to the LogManager (JDK-6865510) as a way to unify failure reporting in the LogManager itself and to deal with failures inside of error manger. As in to remove that ugly System.err code in Handler.reportError. The LogManager would work kind of like Thread.defaultUncaughtExceptionHandler and the Handler.errorManager is kind of like the Thread.uncaughtException handler.
Well. I'm with you on this. I would prefer the fix is a part of more
wider enhancement that unified Logger's inner errors reporting inclusive
redesign for ErrorManager and so on. Probably it's not a big deal, but
it should be some JEP for JDK9. I'm afraid it's too late for 9.
> Then to fix this bug we could report it to via LogManager.getLogManager().reportError.
Well, it's good solution as a default behavior if a user not specified
its own ErrorManager. But, I don't think we have to restrict a user with
what ErrorManager is used in their Formatters. So, set/getErrorManager
for Formatter, Handler and others make sense, i guess.
Ok, I'm still hope on some compromise with the current solution.
The only way to avoid undesirable behavior changes that I see now is
some property in logging.properties that would define if the
ErrorManager in Formatter for a specific Handler must be specified by
default with something. Something like that:
java.util.logging.ConsoleHandler.formatter.ErrorManager = default
If it's set in logging.properties then we are eligible to call
"formatter.setErrorManager(errorManager);" somewhere in Handler
constructor. If not, we have to use Formatter as it has been specified
in specific handler.
Looks ugly, but Let my play with that. If I found it OK, I'll back with
> From: Alexander Fomin <alexander.fomin at oracle.com>
> Sent: Friday, November 20, 2015 11:27 AM
> To: Jason Mehrens; core-libs-dev at openjdk.java.net; Daniel Fuchs; mandy.chung at oracle.com
> Subject: Re: JDK 9 RFR : java.util.logging.Formatter#formatMessage() swallows Exceptions
> Hi Jason
> On 20.11.2015 17:47, Jason Mehrens wrote:
>> Why not just cache the last exception in the formatter and use getTail to clear it and report it?
> It might be a good solution, but according to spec
> Formatter#getTail() method is intended to return the tail string for a
> set of formatted records. With your approach it would need to modify the
> spec any way with notes about possible exception messages in the queue.
> Besides that, the tail of the records should be the latest record.
> I think that according to current spec it must be just that unformatted
> message which had been passed to j.u.l.Formatter#formatMessage() and
> failed to be formatted. Therefore the logic in Handler would be go
> through all records in order to find Exceptions and report them if
> found. Looks a little bit strange.
> But what I really care about is possible compatibility issue if
> getHead()/Tail() overridden in user's Formatter implementation. One of
> the example is j.u.l.XMLFormatter. Even if Exceptions have been cached
> in base Formatter, it would have not been reachable in Handler with
> current XMLFormatter.getTail() implementation. Well, we can modify
> XMLFormatter.getTail() since it's Java code, but I don't think user's
> will happy with an idea to modify anything in their app code.
>> Since formatter is in the same package as Handler you will have elevated access to the error manager through Handler.reportError.
> Well, to which Handler? Formatter don't know anything about Handler(s)
> that use(s) it. So, the only way in Formatter to get the reference on
> some Handler are methods where it passed getHead()/getTail(). Taking
> into account these methods may be overridden in user's implementations
> the solution doesn't seem reliable.
> And then, with setErrorManager()/getErrorManager() user may set his
> favorite ErrorManager.
> Any way, taking into account that at least some spec changes would need
> in any case, it would better to extend API for flexibility and reliability.
>> That also makes it so you don't have to change the public API of Formatter.
>> 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 : java.util.logging.Formatter#formatMessage() swallows Exceptions
>> please, review this patch to report errors in
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8137005
>> Webrev: http://cr.openjdk.java.net/~dfuchs/alexander/8137005/webrev.00
>> 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
>> A couple of new regression tests have been created:
>> test/java/util/logging/Test8137005.java - real case provided by
>> 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:
>> All tests passed passed.
>> failures in the job are known issues and not related to the fix.
More information about the core-libs-dev