RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given
ExceptionInInitializerError(Throwable cause) sets the detail message to null. It'd be helpful to include a detail message rather than null as in Throwable(Throwable cause) that constructs a new throwable with a default message cause==null ? null : cause.toString(). This would help troubleshooting of writing new bootstrap methods where VM currently asserts non-null linkage error message [1] where there are other solutions such as constructing the message in the VM or removing the assert. This patch proposes to change ExceptionInInitializerError(Throwable cause) to default the detail message to cause.toString() if cause is not null. It's an incompatibility change; for example existing code that expects a null message will now see a detail message. But the compatibility risk should be low as EIIE should be fatal. This also proposes to add new LinkageError(Throwable cause) constructor to retrofit the exception chaining mechanism and that allows to clean up the current ExceptionInInitializerError implementation. Webrev: http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.00 I will create a CSR. Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8208172
Hi Mandy, Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField? The main challegne, I think, would be the deserialization of an old ExceptionInInitializerError.exception into new Throwable.cause in case the stream did not have Throwable.cause (which is the case for old ExceptionInInitializerError(s)) Ragards, Peter On 08/15/2018 11:46 PM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message to null. It'd be helpful to include a detail message rather than null as in Throwable(Throwable cause) that constructs a new throwable with a default message cause==null ? null : cause.toString().
This would help troubleshooting of writing new bootstrap methods where VM currently asserts non-null linkage error message [1] where there are other solutions such as constructing the message in the VM or removing the assert.
This patch proposes to change ExceptionInInitializerError(Throwable cause) to default the detail message to cause.toString() if cause is not null. It's an incompatibility change; for example existing code that expects a null message will now see a detail message. But the compatibility risk should be low as EIIE should be fatal.
This also proposes to add new LinkageError(Throwable cause) constructor to retrofit the exception chaining mechanism and that allows to clean up the current ExceptionInInitializerError implementation.
Webrev: http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.00
I will create a CSR.
On 8/15/18 3:20 PM, Peter Levart wrote:
Hi Mandy,
Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField?
Thanks for asking. I meant to mention this and it'd be nice to follow up this in a separate issue. The private exception field exists since 1.1 and kept there for serialization. getException in existing releases returns the exception field. I can't think of any way to remove the exception field in JDK n to deserialize it with older JDK x unless JDK x was changed to write the exception field with the cause or getException to return cause. I think we should remove the ExceptionInInitializerError::getCause method and have getException to return getCause(). I think the simplest is to keep the exception field and make sure it's set with the cause. Existing version of EIIE always has null cause. readObject will set the cause to be same as exception. There may be other options.
The main challegne, I think, would be the deserialization of an old ExceptionInInitializerError.exception into new Throwable.cause in case the stream did not have Throwable.cause (which is the case for old ExceptionInInitializerError(s))
Deserializing from an older EIIE to a newer EIIE is feasible. I was thinking to separate it as a different issue. Mandy
On 8/15/18 5:10 PM, mandy chung wrote:
I think we should remove the ExceptionInInitializerError::getCause method and have getException to return getCause(). I think the simplest is to keep the exception field and make sure it's set with the cause. Existing version of EIIE always has null cause. readObject will set the cause to be same as exception. There may be other options.
I prototype it: http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.01/ I manually verified it reading/writing with new and old version. I will have to add the interop test case and possibly add a package-private method to set Throwable::cause (rather than making the field package-private). Will update you. Mandy
Hello, On 8/15/2018 5:10 PM, mandy chung wrote:
On 8/15/18 3:20 PM, Peter Levart wrote:
Hi Mandy,
Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField?
Thanks for asking. I meant to mention this and it'd be nice to follow up this in a separate issue.
The private exception field exists since 1.1 and kept there for serialization. getException in existing releases returns the exception field. I can't think of any way to remove the exception field in JDK n to deserialize it with older JDK x unless JDK x was changed to write the exception field with the cause or getException to return cause.
Just a quick comment, it is possible, although a bit tedious, to remove a field and retain serial compatibility if readObject/writeObject methods are added to the new version of the class. There are a few examples of doing this kind of conversion in the JDK, such as for BigInteger. HTH, -Joe
On 8/16/18 4:48 PM, joe darcy wrote:
Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField?
Thanks for asking. I meant to mention this and it'd be nice to follow up this in a separate issue.
The private exception field exists since 1.1 and kept there for serialization. getException in existing releases returns the exception field. I can't think of any way to remove the exception field in JDK n to deserialize it with older JDK x unless JDK x was changed to write the exception field with the cause or getException to return cause.
Just a quick comment, it is possible, although a bit tedious, to remove a field and retain serial compatibility if readObject/writeObject methods are added to the new version of the class. There are a few examples of doing this kind of conversion in the JDK, such as for BigInteger.
Thanks Joe. In EIIE case, ideally we should remove the private exception field and then write that to the serialize stream. However, PutField::put requires the field to be present in the current class; otherwise it throws IAE. ObjectOutputStream.PutField fields = s.putFields(); fields.put("exception", getCause()); I haven't digged the history but I assume a field in BigInteger was not renamed/removed. Any other suggestion would be appreciated. Mandy
On 08/17/2018 01:54 AM, mandy chung wrote:
On 8/16/18 4:48 PM, joe darcy wrote:
Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField?
Thanks for asking. I meant to mention this and it'd be nice to follow up this in a separate issue.
The private exception field exists since 1.1 and kept there for serialization. getException in existing releases returns the exception field. I can't think of any way to remove the exception field in JDK n to deserialize it with older JDK x unless JDK x was changed to write the exception field with the cause or getException to return cause.
Just a quick comment, it is possible, although a bit tedious, to remove a field and retain serial compatibility if readObject/writeObject methods are added to the new version of the class. There are a few examples of doing this kind of conversion in the JDK, such as for BigInteger.
Thanks Joe. In EIIE case, ideally we should remove the private exception field and then write that to the serialize stream. However, PutField::put requires the field to be present in the current class; otherwise it throws IAE.
ObjectOutputStream.PutField fields = s.putFields(); fields.put("exception", getCause());
Not necessarily. Check ConcurrentHashMap for example. It contains several "pseudo" fields, declared with the "serialPersistentFields" static final field. The only problem with this approach is that while deserializing the Throwable part of the ExceptionInInitializerError from the serial stream produced by an old version of EIIE, the cause field will already be deserialized as null and later in ExceptionInInitializerError.readObject when "exception" pseudo field is read-in, its value would have to be written over Throwable.cause field, but public API will not allow that (re-initialization of cause is not permitted), so some package-private API would have to be used to overwrite "cause". But this same problem is present even if you keep the physical "exception" field in the ExceptionInInitializerError. You have to be prepared to deserialize an old version of EIIE where the "cause" is null and the "exception" is non-null. Regards, Peter
I haven't digged the history but I assume a field in BigInteger was not renamed/removed.
Any other suggestion would be appreciated.
Mandy
Hi Mandy, This what we were doing in the past and has examples of removing fields with expected tests: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051339.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html However, this case is a little different because it actually disallows subsequent initCause. I would assume that will get tricky if you deserialize the old binary form in a newer JDK because we would have to jump through some hoops to update null to the cause exception. Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of mandy chung <mandy.chung@oracle.com> Sent: Thursday, August 16, 2018 6:54 PM To: joe darcy; Peter Levart Cc: core-libs-dev Subject: Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given On 8/16/18 4:48 PM, joe darcy wrote:
Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField?
Thanks for asking. I meant to mention this and it'd be nice to follow up this in a separate issue.
The private exception field exists since 1.1 and kept there for serialization. getException in existing releases returns the exception field. I can't think of any way to remove the exception field in JDK n to deserialize it with older JDK x unless JDK x was changed to write the exception field with the cause or getException to return cause.
Just a quick comment, it is possible, although a bit tedious, to remove a field and retain serial compatibility if readObject/writeObject methods are added to the new version of the class. There are a few examples of doing this kind of conversion in the JDK, such as for BigInteger.
Thanks Joe. In EIIE case, ideally we should remove the private exception field and then write that to the serialize stream. However, PutField::put requires the field to be present in the current class; otherwise it throws IAE. ObjectOutputStream.PutField fields = s.putFields(); fields.put("exception", getCause()); I haven't digged the history but I assume a field in BigInteger was not renamed/removed. Any other suggestion would be appreciated. Mandy
Hi Peter, Jason, Joe, Thanks for the pointers. I have missed the use of serialPersistentFields (thanks to Peter for calling this out). I read the javadoc and serialization spec but that didn't come clearly to me. It'd be good to include an example in the javadoc (will file a JBS issue). W.r.t. the package-private API to set the cause in EIIE [1], yes it's needed and it's done in webrev.01. Let me redo the readObject/writeObject part and send a new webrev. Mandy http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.01/src/java.... On 8/17/18 8:05 AM, Jason Mehrens wrote:
Hi Mandy,
This what we were doing in the past and has examples of removing fields with expected tests: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051339.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html > However, this case is a little different because it actually disallows subsequent initCause. I would assume that will get tricky if you deserialize the old binary form in a newer JDK because we would have to jump through some hoops to update null to the cause exception.
Jason
Hi Mandy, On 08/17/2018 05:20 PM, mandy chung wrote:
Hi Peter, Jason, Joe,
Thanks for the pointers. I have missed the use of serialPersistentFields (thanks to Peter for calling this out). I read the javadoc and serialization spec but that didn't come clearly to me. It'd be good to include an example in the javadoc (will file a JBS issue).
W.r.t. the package-private API to set the cause in EIIE [1], yes it's needed and it's done in webrev.01. Let me redo the readObject/writeObject part and send a new webrev.
Mandy http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.01/src/java....
I think it would be nicer to keep all the logic in one place (EIIE.readObject()) and make Throwable.setCause() plain unconditional setter. For example: void readObject(.... Throwable exception = ... if (getCause() == null && exception != null) { setCause(exception); } Regards, Peter
On 8/17/18 8:05 AM, Jason Mehrens wrote:
Hi Mandy,
This what we were doing in the past and has examples of removing fields with expected tests: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051339.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
However, this case is a little different because it actually disallows subsequent initCause. I would assume that will get tricky if you deserialize the old binary form in a newer JDK because we would have to jump through some hoops to update null to the cause exception.
Jason
On 8/16/18 4:48 PM, joe darcy wrote:
On 8/15/2018 5:10 PM, mandy chung wrote:
On 8/15/18 3:20 PM, Peter Levart wrote:
Hi Mandy,
Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField?
Thanks for asking. I meant to mention this and it'd be nice to follow up this in a separate issue.
The private exception field exists since 1.1 and kept there for serialization. getException in existing releases returns the exception field. I can't think of any way to remove the exception field in JDK n to deserialize it with older JDK x unless JDK x was changed to write the exception field with the cause or getException to return cause.
Just a quick comment, it is possible, although a bit tedious, to remove a field and retain serial compatibility if readObject/writeObject methods are added to the new version of the class. There are a few examples of doing this kind of conversion in the JDK, such as for BigInteger.
Looking at the history, it turns out that these redundant fields were removed at one point when the exception chaining support was initially implemented but it got reverted to fix JDK-4385429. I have posted a proposed patch to remove the private Throwable exception field and also clean up a few other exception classes as a separate JBS issue. http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055415.h... Mandy
Hi Mandy, Like in previous patches, I advocated for using 'super.getCause()' in writeObject to avoid any subclass hooking into serialization. I even lean towards the legacy getXXX methods call super.getCause too so they are compatible with previous behavior. Does Throwable.setCause need a more obscure name encase there are subclasses of throwable in the wild with that signature? EIIE has double semi-colon in the constructor and PAE has a random javadoc modification. Cheers, Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of mandy chung <mandy.chung@oracle.com> Sent: Thursday, September 13, 2018 1:49 PM To: joe darcy; Peter Levart Cc: core-libs-dev Subject: Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given On 8/16/18 4:48 PM, joe darcy wrote:
On 8/15/2018 5:10 PM, mandy chung wrote:
On 8/15/18 3:20 PM, Peter Levart wrote:
Hi Mandy,
Just a question. Why does "private Throwable exception" field in ExceptionInInitializerError exist? Was it there before there was a "cause" in Throwable and later still remained there because of serialization format? Would it be possible to "simulate" its effect for serialization using "serialPersistentFields" and ObjectOutputStream.PutField?
Thanks for asking. I meant to mention this and it'd be nice to follow up this in a separate issue.
The private exception field exists since 1.1 and kept there for serialization. getException in existing releases returns the exception field. I can't think of any way to remove the exception field in JDK n to deserialize it with older JDK x unless JDK x was changed to write the exception field with the cause or getException to return cause.
Just a quick comment, it is possible, although a bit tedious, to remove a field and retain serial compatibility if readObject/writeObject methods are added to the new version of the class. There are a few examples of doing this kind of conversion in the JDK, such as for BigInteger.
Looking at the history, it turns out that these redundant fields were removed at one point when the exception chaining support was initially implemented but it got reverted to fix JDK-4385429. I have posted a proposed patch to remove the private Throwable exception field and also clean up a few other exception classes as a separate JBS issue. http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055415.h... Mandy
On 9/13/18 12:47 PM, Jason Mehrens wrote:
Hi Mandy,
Like in previous patches, I advocated for using 'super.getCause()' in writeObject to avoid any subclass hooking into serialization. I even lean towards the legacy getXXX methods call super.getCause too so they are compatible with previous behavior.
Good point. It's probably rare to have subclasses of these types and overrides getCause method. In any case I agree it should call super.getCause for correctness. I also added a test case with overridden getCause method. http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8210721/webrev.01/
Does Throwable.setCause need a more obscure name encase there are subclasses of throwable in the wild with that signature?
This is package-private method.
EIIE has double semi-colon in the constructor and PAE has a random javadoc modification.
Good catch! It's cleaned up. Mandy
Looks good. I like the change of making setCause final. Jason ________________________________________ From: mandy chung <mandy.chung@oracle.com> Sent: Thursday, September 13, 2018 3:50 PM To: Jason Mehrens; joe darcy; Peter Levart Cc: core-libs-dev Subject: Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given On 9/13/18 12:47 PM, Jason Mehrens wrote: Hi Mandy, Like in previous patches, I advocated for using 'super.getCause()' in writeObject to avoid any subclass hooking into serialization. I even lean towards the legacy getXXX methods call super.getCause too so they are compatible with previous behavior. Good point. It's probably rare to have subclasses of these types and overrides getCause method. In any case I agree it should call super.getCause for correctness. I also added a test case with overridden getCause method. http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8210721/webrev.01/ Does Throwable.setCause need a more obscure name encase there are subclasses of throwable in the wild with that signature? This is package-private method. EIIE has double semi-colon in the constructor and PAE has a random javadoc modification. Good catch! It's cleaned up. Mandy
Hi Mandy, Not a review - sorry :) On 16/08/2018 7:46 AM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message to null. It'd be helpful to include a detail message rather than null as in Throwable(Throwable cause) that constructs a new throwable with a default message cause==null ? null : cause.toString().
This would help troubleshooting of writing new bootstrap methods where VM currently asserts non-null linkage error message [1] where there are other solutions such as constructing the message in the VM or removing the assert.
I think that's just a bug in the VM code making inappropriate assumptions! There shouldn't have to be a detail message, especially in a wrapping exception like ExceptionInInitializerError! It only gets thrown because some other exception was thrown, there's really no more detail for the EIIE itself. Promoting the cause's detail message to be the detail message of the EIIE just seems wrong to me. Did this come about through the new code that saves the original cause of the EIIE so that it can be reported when the subsequent NoClassDefFoundError is thrown due to the class being in the erroneous state? (I don't recall if that actually got pushed.) David
This patch proposes to change ExceptionInInitializerError(Throwable cause) to default the detail message to cause.toString() if cause is not null. It's an incompatibility change; for example existing code that expects a null message will now see a detail message. But the compatibility risk should be low as EIIE should be fatal.
This also proposes to add new LinkageError(Throwable cause) constructor to retrofit the exception chaining mechanism and that allows to clean up the current ExceptionInInitializerError implementation.
Webrev: http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.00
I will create a CSR.
On 8/15/18 11:00 PM, David Holmes wrote:
Hi Mandy,
Not a review - sorry :)
On 16/08/2018 7:46 AM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message to null. It'd be helpful to include a detail message rather than null as in Throwable(Throwable cause) that constructs a new throwable with a default message cause==null ? null : cause.toString().
This would help troubleshooting of writing new bootstrap methods where VM currently asserts non-null linkage error message [1] where there are other solutions such as constructing the message in the VM or removing the assert.
I think that's just a bug in the VM code making inappropriate assumptions! There shouldn't have to be a detail message, especially in a wrapping exception like ExceptionInInitializerError! It only gets thrown because some other exception was thrown, there's really no more detail for the EIIE itself. Promoting the cause's detail message to be the detail message of the EIIE just seems wrong to me.
I think even VM removes the assert, I don't see how EIIE includes the detail message is wrong? It's consistent with the Throwable(Throwable cause) constructor.
Did this come about through the new code that saves the original cause of the EIIE so that it can be reported when the subsequent NoClassDefFoundError is thrown due to the class being in the erroneous state? (I don't recall if that actually got pushed.)
I am not sure but I don't think so. I ran into this when I develop the bootstrap methods. I think this is a good EIIE clean up. Mandy
On 17/08/2018 1:18 AM, mandy chung wrote:
On 8/15/18 11:00 PM, David Holmes wrote:
Hi Mandy,
Not a review - sorry :)
On 16/08/2018 7:46 AM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message to null. It'd be helpful to include a detail message rather than null as in Throwable(Throwable cause) that constructs a new throwable with a default message cause==null ? null : cause.toString().
This would help troubleshooting of writing new bootstrap methods where VM currently asserts non-null linkage error message [1] where there are other solutions such as constructing the message in the VM or removing the assert.
I think that's just a bug in the VM code making inappropriate assumptions! There shouldn't have to be a detail message, especially in a wrapping exception like ExceptionInInitializerError! It only gets thrown because some other exception was thrown, there's really no more detail for the EIIE itself. Promoting the cause's detail message to be the detail message of the EIIE just seems wrong to me.
I think even VM removes the assert, I don't see how EIIE includes the detail message is wrong? It's consistent with the Throwable(Throwable cause) constructor.
I think it is wrong because the message is not that of the EIIE but that of the original exception. If EIIE must have a detail message then that should just be something explicit like: Caused by <cause exception type> : <cause detail message> Unlike general Throwables that may or may not have a cause, a true EIIE must always have a cause when thrown by the JVM. David -----
Did this come about through the new code that saves the original cause of the EIIE so that it can be reported when the subsequent NoClassDefFoundError is thrown due to the class being in the erroneous state? (I don't recall if that actually got pushed.)
I am not sure but I don't think so. I ran into this when I develop the bootstrap methods. I think this is a good EIIE clean up.
Mandy
Hi David, Exceptions should have a detail message that is useful. For EIIE, all of the salient information is in the message of the cause and is much more useful that something generic that just echos the EIIE's type. Without a stacktrace EIIE, with a generic message is nearly useless. Generally, where an exception is caught just to be wrapped and propagated with a different type, the message from the underlying cause is more useful to report. $.02, Roger On 8/16/18 6:22 PM, David Holmes wrote:
On 17/08/2018 1:18 AM, mandy chung wrote:
On 8/15/18 11:00 PM, David Holmes wrote:
Hi Mandy,
Not a review - sorry :)
On 16/08/2018 7:46 AM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message to null. It'd be helpful to include a detail message rather than null as in Throwable(Throwable cause) that constructs a new throwable with a default message cause==null ? null : cause.toString().
This would help troubleshooting of writing new bootstrap methods where VM currently asserts non-null linkage error message [1] where there are other solutions such as constructing the message in the VM or removing the assert.
I think that's just a bug in the VM code making inappropriate assumptions! There shouldn't have to be a detail message, especially in a wrapping exception like ExceptionInInitializerError! It only gets thrown because some other exception was thrown, there's really no more detail for the EIIE itself. Promoting the cause's detail message to be the detail message of the EIIE just seems wrong to me.
I think even VM removes the assert, I don't see how EIIE includes the detail message is wrong? It's consistent with the Throwable(Throwable cause) constructor.
I think it is wrong because the message is not that of the EIIE but that of the original exception.
If EIIE must have a detail message then that should just be something explicit like:
Caused by <cause exception type> : <cause detail message>
Unlike general Throwables that may or may not have a cause, a true EIIE must always have a cause when thrown by the JVM.
David -----
Did this come about through the new code that saves the original cause of the EIIE so that it can be reported when the subsequent NoClassDefFoundError is thrown due to the class being in the erroneous state? (I don't recall if that actually got pushed.)
I am not sure but I don't think so. I ran into this when I develop the bootstrap methods. I think this is a good EIIE clean up.
Mandy
Hi Mandy, A correction on my part ... On 17/08/2018 8:22 AM, David Holmes wrote:
On 17/08/2018 1:18 AM, mandy chung wrote:
On 8/15/18 11:00 PM, David Holmes wrote:
Hi Mandy,
Not a review - sorry :)
On 16/08/2018 7:46 AM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message to null. It'd be helpful to include a detail message rather than null as in Throwable(Throwable cause) that constructs a new throwable with a default message cause==null ? null : cause.toString().
This would help troubleshooting of writing new bootstrap methods where VM currently asserts non-null linkage error message [1] where there are other solutions such as constructing the message in the VM or removing the assert.
I think that's just a bug in the VM code making inappropriate assumptions! There shouldn't have to be a detail message, especially in a wrapping exception like ExceptionInInitializerError! It only gets thrown because some other exception was thrown, there's really no more detail for the EIIE itself. Promoting the cause's detail message to be the detail message of the EIIE just seems wrong to me.
I think even VM removes the assert, I don't see how EIIE includes the detail message is wrong? It's consistent with the Throwable(Throwable cause) constructor.
I think it is wrong because the message is not that of the EIIE but that of the original exception.
If EIIE must have a detail message then that should just be something explicit like:
Caused by <cause exception type> : <cause detail message>
This is of course what you actually propose by using cause.toString() - for some reason I was thinking you were just using the cause's detail message as the EIIE detail message. Sorry about that. I still don't think this change is actually necessary, but it's fine. Thanks, David -----
Unlike general Throwables that may or may not have a cause, a true EIIE must always have a cause when thrown by the JVM.
David -----
Did this come about through the new code that saves the original cause of the EIIE so that it can be reported when the subsequent NoClassDefFoundError is thrown due to the class being in the erroneous state? (I don't recall if that actually got pushed.)
I am not sure but I don't think so. I ran into this when I develop the bootstrap methods. I think this is a good EIIE clean up.
Mandy
On 8/20/18 6:06 PM, David Holmes wrote:
If EIIE must have a detail message then that should just be something explicit like:
Caused by <cause exception type> : <cause detail message>
This is of course what you actually propose by using cause.toString() - for some reason I was thinking you were just using the cause's detail message as the EIIE detail message. Sorry about that.
Yup, the proposed detail message for EIIE(Throwable cause) constructor will have: <cause exception type> : <cause detail message>
I still don't think this change is actually necessary, but it's fine.
I see it's a small improvement for troubleshooting even people live without it. A related but different topic: While digging up the exception chaining facility related issue, I found that the private exception field was removed initially but added back because of JDK-4385429 [1] that failed to deserialize since the cause can't be re-initialized. If I redo the fix for JDK-4385429 (essentially adding the package- private Throwable::setCause), several other exception types such as UndeclaredThrowableException and InvocationTargetException will get the default detail message rather than null. Anyway, just to mention that a few other exception types may have similar change. Mandy [1] https://bugs.openjdk.java.net/browse/JDK-4385429
Thanks, David
participants (6)
-
David Holmes
-
Jason Mehrens
-
joe darcy
-
mandy chung
-
Peter Levart
-
Roger Riggs