Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
Alexander, >What is wrong with get/serErrorManager for Formatter? It should be just >a way for Formatter to report its internal errors. 1. You can't inherit the Handler error manager until after construction of the formatter. That means the concept of inheriting an error manager from a handler is flawed. Forcing subclasses to include an ErrorManager or Handler constructor is not very clean either. Especially if the formatter already has constructors that take multiple arguments. 2. Formatters can decorate other formatters. See point #1. Logging does not allow for creating an ErrorManager that is hostile that allows an exception to escape (without resorting to hacks). Making the internal and external formatter work as one unit gets messy. 3. How do you control System.err chatter? It used to be that every handler had an error manager and every error manager could report one problem. If we increase the number of error managers we can increase the amount of chatter. 3. The JDK Handlers already expect that formatters throw exceptions see (StreamHandler). Publishing is where things are expected to fail. Everything that happens upstream from a Handler is expected not to fail. It is already cleanly established that the handler should be the mediator between the formatter and the error manager. The creation of a handler creates the error manager and formatters so it is much cleaner to have the formatter simply throw exceptions to the handler and have the handler delegate to the error manager. 4. ErrorManagers installed on formatters would mainly deal with programing bugs (pattern syntax errors and broken Object.toString implementations) not expected failures like I/O. Seems excessive to add an ErrorManager that conditionally handles bugs. Handers on the other hand can fail with checked exceptions like I/O which is a different class of errors. 5. ErrorManager properties on a Formatter is more API complexity and more mutable state to deal with and test. I think API producers and API consumers would like to avoid this debt. 6. ErrorManager defines an error code as an argument. If objects belonging to a Handler were supposed to have an ErrorManager we wouldn't need the code argument. This new addtion conflicts with the original design. 7. It is wasted object. Not all Handlers have just one formatter and those formatters can contain formatters. There are two clean ways errors should be reported in a formatter without changing the API. Either throw the exception to the caller (Handler) or format the error using a best effort and return it as a string. Going back to https://bugs.openjdk.java.net/browse/JDK-8028233, the pain point is that the error is lost when we create buggy Object.toString implementations. A very clean solution would be to make the formatter simply format the exception, the pattern syntax (record.getMessage), the types of the arguments (avoids calling overridden toString methods) and return a concatinated string. This gets around the limitation of reporting only one failure and is not hostile which is nice for compatiblity. Since we are dealing with formatMessage, changing the behavior doesn't break XMLFormatter or SimpleFormatter because that returned string is free form. All the API says is "returns a localized and formatted message" when faced with a bad command (pattern syntax error) it would be legal to alter the string returned in the failure case. Jason
Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
Hi Jason On 20.11.2015 22:15, Jason Mehrens wrote: Alexander, 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 yet. 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 behavior. - 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 ignored. 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 new webrev. Thanks, Alexander Jason From: Alexander Fomin Sent: Friday, November 20, 2015 11:27 AM To: Jason Mehrens; core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@oracle.com Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions Hi Jason On 20.11.2015 17:47, Jason Mehrens wrote: Alexander, Why not just cache the last excep
Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
Alexander,
Given those constraints, what about simply modify the output string? As in:
=
.
} catch (Exception ex) {
// Formatting failed: use localized format string.
return format + " " + toString(record, ex);
}
private String toString(LogRecord record, Exception ex) {
StringWriter sw = new StringWriter();
PrintWriter pw = new PrintWriter(sw);
Object[] params = record.getParameters();
if (params != null) {
String sep = "";
for (Object o : params) {
pw.append(sep);
if (o != null) {
pw.append(o.getClass().getName());
} else {
pw.append("null");
}
pw.println();
sep = ",";
}
}
ex.printStackTrace(pw);
return sw.toString();
}
There is wiggle room in the API docs for modifying the output string which
would avoid a CCC.
Jason
From: Alexander Fomin
Sent: Monday, November 23, 2015 9:27 AM
To: Jason Mehrens; core-libs-dev@openjdk.java.net; Daniel Fuchs;
mandy.ch...@oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage()
swallows Exceptions
Hi Jason
On 20.11.2015 22:15, Jason Mehrens wrote:
> Alexander,
>
> 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
yet.
>
> 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
behavior.
- 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
ignored.
>
> 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
Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
Hi Jason On 20.11.2015 17:47, Jason Mehrens wrote: Alexander, 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. Thanks, Alexander That also makes it so you don't have to change the public API of Formatter. Jason From: core-libs-dev on behalf of Alexander Fomin Sent: Friday, November 20, 2015 7:48 AM To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@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
Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
Alexander, 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). 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? 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. 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. Then to fix this bug we could report it to via LogManager.getLogManager().reportError. Jason From: Alexander Fomin Sent: Friday, November 20, 2015 11:27 AM To: Jason Mehrens; core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@oracle.com Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions Hi Jason On 20.11.2015 17:47, Jason Mehrens wrote: > Alexander, > > 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. Thanks, Alexander > That also makes it so you don't have to change the public API of Formatter. > > Jason > > > From: core-libs-dev on behalf of > Alexander Fomin > Sent: Friday, November 20, 2015 7:48 AM > To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@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
Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
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
Sent: Friday, November 20, 2015 11:04 AM
To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net;
mandy.ch...@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
> Sent: Friday, November 20, 2015 9:32 AM
> To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net;
> mandy.ch...@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 on behalf of
>> Alexander Fomin
>> Sent: Friday, November 20, 2015 7:48 AM
>> To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@oracle.com
>>
Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
On 20.11.2015 20:04, Daniel Fuchs wrote: 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? What about MemoryHandler and SocketHandler? Should we call getHead()/getTail() somewhere in it just to get a Formatter a chance to report errors? Thanks, Alexander 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 Sent: Friday, November 20, 2015 9:32 AM To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net; mandy.ch...@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 on behalf of Alexander Fomin Sent: Friday, November 20, 2015 7:48 AM To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@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-
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 Sent: Friday, November 20, 2015 9:32 AM To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net; mandy.ch...@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 on behalf of Alexander Fomin Sent: Friday, November 20, 2015 7:48 AM To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@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
Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions
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). 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 Sent: Friday, November 20, 2015 9:32 AM To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net; mandy.ch...@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 on behalf of > Alexander Fomin > Sent: Friday, November 20, 2015 7:48 AM > To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@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 > > >
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 on behalf of Alexander Fomin Sent: Friday, November 20, 2015 7:48 AM To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@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
