[15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace
David Holmes
david.holmes at oracle.com
Tue Jul 14 23:09:42 UTC 2020
Hi Goetz,
On 14/07/2020 11:48 pm, Lindenmaier, Goetz wrote:
> Hi,
>
> Yes, Coleen, you are right. We must preserve the lazy
> computation, and also reduce overhead on discarded
> exceptions.
>
> And yes, we can do it with a counter:
> http://cr.openjdk.java.net/~goetz/wr20/8248476-NPE_fillInStackTrace-jdk15/06/
This is my preferred approach. It would be nice if we could use a
boolean instead of a counter, but I think we have a ternary state we
need to track. The counter approach could be made more state-based (and
avoid theoretical overflow problem) as follows:
public synchronized Throwable fillInStackTrace() {
if (numStackTracesFilledIn == 0) {
numStackTracesFilledIn = 1;
} else if (numStackTracesFilledIn == 1) {
// If the stack trace is changed the extended NPE algorithm
// will compute a wrong message. So compute it beforehand.
extendedMessage = getExtendedNPEMessage();
numStackTracesFilledIn = 2;
}
return super.fillInStackTrace();
}
Note neither new field needs to be volatile as Remi pointed out.
> but I would prefer placeholder strings:
> http://cr.openjdk.java.net/~goetz/wr20/8248476-NPE_fillInStackTrace-jdk15/07/
> This way we need only one new field.
I found the string version awkward to read, sorry.
I don't think the extra field for the counter is a concern here.
Thanks,
David
-----
> (I need two placeholders, because the getExtendedNPEMessage0()
> sometimes returns null. If I write null into the extendedMessage field,
> fillInStackTrace sets it to mustComputeExtendedNPEMessage a second
> time.)
>
> With webrev 07 the overhead on discarded exceptions is basically the
> same as with webrev 05: one additional field, one assignment in fillInStackTrace().
>
> What do you think?
>
> Best regards,
> Goetz.
>
>
>
>
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Tuesday, July 14, 2020 1:55 PM
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'forax at univ-mlv.fr'
>> <forax at univ-mlv.fr>; Alan Bateman <Alan.Bateman at oracle.com>
>> Cc: Christoph Dreis <christoph.dreis at freenet.de>; hotspot-runtime-dev
>> <hotspot-runtime-dev at openjdk.java.net>
>> Subject: Re: [15] RFR: 8248476: No helpful NullPointerException message
>> after calling fillInStackTrace
>>
>> Correction ...
>>
>> On 14/07/2020 12:11 pm, David Holmes wrote:
>>> Hi Goetz,
>>>
>>> Okay ... if I understand your position correctly you are looking at this
>>> as if the extended message is created at the time the NPE is thrown, and
>>> it is an implementation detail that we actually determine it lazily. If
>>> it were eagerly determined then neither fillInstacktrace() nor
>>> setStackTrace() would make any difference to the message - just as with
>>> any other exception message.
>>>
>>> However, the lazy determination of the message causes a problem with
>>> fillInStackTrace() because that call will destroy the original backtrace
>>> needed to produce the original message, and create an incorrect message.
>>> setStackTrace() does not have a similar problem because, simply by the
>>> way the current implementation works it doesn't touch the original
>>> backtrace.
>>>
>>> So you are proposing to only fix the bug that is evident in relation to
>>> fillInStackTrace() by no longer evaluating the extended message if
>>> fillInStackTrace() is called after the NPE was constructed.
>>>
>>> But in doing so you break the illusion that the extended message acts
>>> as-if determined at construction time, because you now effectively clear
>>> it when fillInStackTrace is called.
>>>
>>> My position was that if fillInStackTrace can be seen to clear it, then
>>> setStackTrace (which is logically somewhat equivalent) should also be
>>> seen to clear it.
>>>
>>> Alternatively, add a new field to NPE to cache the extended error
>>> message, and explicitly evaluate the message if fillInStackTrace() is
>>> called. That will continue the illusion that the extended message was
>>> actually set at construction time. No changes needed to setStackTrace()
>>> as we can still lazily compute the extended message.
>>>
>>> Something like:
>>>
>>> private String extendedMessage;
>>>
>>> public synchronized Throwable fillInStackTrace() {
>>> if (extendedMessage == NULL) {
>>> extendedMessage = getExtendedNPEMessage();
>>> }
>>> return super.fillInStackTrace();
>>> }
>>
>> Coleen pointed out to me that we can't do it like this because we need
>> the initial fillInStacktrace to be fast and we want the extended message
>> computed lazily. So it will still need a counter so we only do this on
>> the second call.
>>
>>
>> private String extendedMessage;
>> private int fillInCount;
>>
>> public synchronized Throwable fillInStackTrace() {
>> if (extendedMessage == NULL && (fillInCount++ == 1)) {
>> extendedMessage = getExtendedNPEMessage();
>> }
>> return super.fillInStackTrace();
>> }
>>
>> or something to that effect.
>>
>> David
>> -----
>>
>>> public String getMessage() {
>>> String message = super.getMessage();
>>> synchronized(this) {
>>> if (message == null) {
>>> // This NPE should have an extended message.
>>> if (extendedMessage == NULL) {
>>> extendedMessage = getExtendedNPEMessage();
>>> }
>>> message = extendedMessage;
>>> }
>>> }
>>> return message;
>>> }
>>>
>>> Cheers,
>>> David
>>>
>>> On 14/07/2020 12:48 am, Lindenmaier, Goetz wrote:
>>>> Hi David,
>>>>
>>>>> Your extended message is only computed when there is no original
>>>>> message.
>>>> Hmm. I would say the extended message is only computed when
>>>> The NPE was raised by the runtime. It happens to never have a
>>>> message so far in these cases.
>>>> But this is two views to the same thing ��
>>>>
>>>>> You're concerned about this scenario:
>>>>>
>>>>> catch (NullPointerException npe) {
>>>>> String msg1 = npe.getMessage(); // gets extends NPE message
>>>>> npe.setStackTrace(...);
>>>>> String msg2 = npe.getMessage(); // gets null
>>>>> }
>>>>>
>>>>> While I find it hard to imagine anyone doing this
>>>> Well, all the scenario are quite artificial:
>>>> - why would you call fillInStackTrace on an exception thrown by the VM?
>>>> - why would you call setStackTrace at all?
>>>>> you can easily have
>>>>> specified that the extended message is only available with the original
>>>>> stacktrace, hence after a second call to fillInStackTrace, or a call to
>>>>> setStackTrace, then the message reverts to being empty.
>>>> The message is not meant to be a special thing that behaves different
>>>> from other messages. Like sometime be available, sometime not.
>>>> It ended up being different through requirements during the
>>>> review.
>>>>
>>>>> To me that makes
>>>>> far more sense than having msg2 continue to report the extended info
>> for
>>>>> the original stacktrace when it now has a new stacktrace.
>>>>>
>>>>> I'm really not seeing why calling fillInstackTrace() a second time
>>>>> should be treated any differently to calling setStackTrace(). They
>>>>> should be handled consistently IMO.
>>>> But then you treat setStackTrace() differently from setStackTrace()
>>>> with other exceptions.
>>>> The reason to treat fillInStackTrace differently is that we lost
>>>> information
>>>> needed to compute it. This is not the case with setStackTrace().
>>>>
>>>> A different solution, the one I would have proposed if I had not
>>>> considered previous comments from reviews, would be to just
>>>> compute the message in the runtime in the call of fillInStackTrace
>>>> before the old stack trace is lost and assign it to the message field.
>>>> This way it would behave similar to all other exceptions. The message
>>>> would just be there ... just that it's computed lazily.
>>>> The cost of the algorithm wouldn't harm that much as other costly
>>>> algorithms (walking the stack) are performed at this point, too.
>>>>
>>>>> We are not talking about all exceptions only about your NPE extended
>>>>> error message.
>>>> Hmm, the inconsistency caused by the code you posted above
>>>> holds for all exceptions. If you fiddle with the stack trace,
>>>> the message might become pointless. Wrt. setStackTrace
>>>> they all behave the same.
>>>> Wrt. fillInStackTrace the message will be wrong. Only this
>>>> needs to be fixed.
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> I implemented an example where wrong stack traces are
>>>>>> printed with LinkageError and NPE, modifying a jtreg test:
>>>>>> http://cr.openjdk.java.net/~goetz/wr20/8248476-
>> NPE_fillInStackTrace-
>>>>> jdk15/05/mess_with_exceptions.patch
>>>>>> See also the generated output added to a comment in the patch.
>>>>>> If the NEP message text was missing in the second printout, I think
>>>>>> this really would be unexpected.
>>>>>> Please note that the correct message is printed after messing
>>>>>> with the stack trace, it's the stack trace that is wrong.
>>>>>> (Not as with the problem I am fixing here where a wrong
>>>>>> message is printed.)
>>>>>>
>>>>>> Best regards,
>>>>>> Goetz.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> I guess the normal usecase of setStackTrace is the other way around:
>>>>>>>> Change the message and throw a new exception with the existing
>>>>>>>> stack trace:
>>>>>>>>
>>>>>>>> try {
>>>>>>>> a.x;
>>>>>>>> catch (NullPointerException e) {
>>>>>>>> throw new NullPointerException("My own error
>>>>>>> message").setStackTrace(e.getStackTrace);
>>>>>>>> }
>>>>>>>>
>>>>>>>> And not taking an arbitrary stack trace and put it into an exception
>>>>>>>> with existing message.
>>>>>>>
>>>>>>> Interesting usage.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Goetz.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>>>> Sent: Friday, July 3, 2020 9:30 AM
>>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
>> 'forax at univ-
>>>>>>> mlv.fr'
>>>>>>>>> <forax at univ-mlv.fr>; Alan Bateman <Alan.Bateman at oracle.com>
>>>>>>>>> Cc: Christoph Dreis <christoph.dreis at freenet.de>;
>>>>>>>>> hotspot-runtime-dev
>>>>>>>>> <hotspot-runtime-dev at openjdk.java.net>
>>>>>>>>> Subject: Re: [15] RFR: 8248476: No helpful NullPointerException
>>>>> message
>>>>>>>>> after calling fillInStackTrace
>>>>>>>>>
>>>>>>>>> Hi Goetz,
>>>>>>>>>
>>>>>>>>> On 3/07/2020 4:32 pm, Lindenmaier, Goetz wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>>> True. To ensure you process the original backtrace only you
>>>>>>>>>>> need to
>>>>>>> add
>>>>>>>>>>> synchronization in getMessage():
>>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr20/8248476-
>>>>> NPE_fillInStackTrace-
>>>>>>>>> jdk15/05/
>>>>>>>>>>
>>>>>>>>>> I added the volatile, too, but as I understand the synchronized
>>>>>>>>>> block brings sufficient memory barriers that this also works
>>>>>>>>>> without.
>>>>>>>>>
>>>>>>>>> No "volatile" needed, or wanted, when all access is within
>>>>>>>>> synchronized
>>>>>>>>> regions.
>>>>>>>>>
>>>>>>>>>>> To be honest the idea that someone would share an exception
>>>>> instance
>>>>>>>>> and
>>>>>>>>>>> concurrently mutate it with fillInStackTrace() whilst printing out
>>>>>>>>>>> information about it just seems highly unrealistic.
>>>>>>>>>> Yes, contention here is quite unlikely, so it should not harm
>>>>> performance
>>>>>>>>> ��
>>>>>>>>>
>>>>>>>>> Contention was not my concern at all. :)
>>>>>>>>>
>>>>>>>>>>> Though after looking at comments in the test I would also
>>>>>>>>>>> suggest that setStackTrace be updated:
>>>>>>>>>> The test shows that after setStackTrace still the correct message
>>>>>>>>>> is computed. This is because the algorithm uses
>>>>>>>>>> Throwable::backtrace
>>>>>>>>>> and not Throwable::stacktrace. Throwable::backtrace is not
>>>>>>>>>> affected by setStackTrace.
>>>>>>>>>> The behavior is just as with any exception. If you fiddle
>>>>>>>>>> with the stack trace, but don't adapt the message text,
>>>>>>>>>> the message might refer to other code than the stack trace
>>>>>>>>>> points to.
>>>>>>>>>
>>>>>>>>> But you can't adapt the message text - there is no setMessage! If
>>>>>>>>> the
>>>>>>>>> message is NULL and you call setStackTrace() then getMessage(), it
>>>>> makes
>>>>>>>>> no sense to return the extended error message that was associated
>>>>> with
>>>>>>>>> the original stack/backtrace.
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Goetz.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>>>>>> Sent: Friday, July 3, 2020 3:37 AM
>>>>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
>>>>> 'forax at univ-
>>>>>>>>> mlv.fr'
>>>>>>>>>>> <forax at univ-mlv.fr>; Alan Bateman <Alan.Bateman at oracle.com>
>>>>>>>>>>> Cc: Christoph Dreis <christoph.dreis at freenet.de>; hotspot-
>> runtime-
>>>>> dev
>>>>>>>>>>> <hotspot-runtime-dev at openjdk.java.net>
>>>>>>>>>>> Subject: Re: [15] RFR: 8248476: No helpful NullPointerException
>>>>>>> message
>>>>>>>>>>> after calling fillInStackTrace
>>>>>>>>>>>
>>>>>>>>>>> Hi Goetz,
>>>>>>>>>>>
>>>>>>>>>>> On 3/07/2020 5:30 am, Lindenmaier, Goetz wrote:
>>>>>>>>>>>> Hi Remi,
>>>>>>>>>>>>
>>>>>>>>>>>> But how does volatile help?
>>>>>>>>>>>> I see the test for numStackTracesFilledIn == 1 then gets
>>>>>>>>>>>> always the
>>>>>>>>>>>> right value.
>>>>>>>>>>>> But the backtrace may not be changed until I read it in
>>>>>>>>>>>> getExtendedNPEMessage. The other thread could change it
>> after
>>>>>>>>>>>> checking numStackTracesFilledIn and before I read the
>> backtrace.
>>>>>>>>>>>
>>>>>>>>>>> True. To ensure you process the original backtrace only you
>>>>>>>>>>> need to
>>>>>>> add
>>>>>>>>>>> synchronization in getMessage():
>>>>>>>>>>>
>>>>>>>>>>> public String getMessage() {
>>>>>>>>>>> String message = super.getMessage();
>>>>>>>>>>> // If the stack trace was changed the extended
>>>>>>>>>>> NPE algorithm
>>>>>>>>>>> // will compute a wrong message.
>>>>>>>>>>> + synchronized(this) {
>>>>>>>>>>> ! if (message == null && numStackTracesFilledIn ==
>>>>>>>>>>> 1) {
>>>>>>>>>>> ! return getExtendedNPEMessage();
>>>>>>>>>>> ! }
>>>>>>>>>>> + }
>>>>>>>>>>> return message;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> To be honest the idea that someone would share an exception
>>>>> instance
>>>>>>>>> and
>>>>>>>>>>> concurrently mutate it with fillInStackTrace() whilst printing out
>>>>>>>>>>> information about it just seems highly unrealistic. But the
>>>>>>>>>>> above fixes
>>>>>>>>>>> it simply. Though after looking at comments in the test I would
>>>>>>>>>>> also
>>>>>>>>>>> suggest that setStackTrace be updated:
>>>>>>>>>>>
>>>>>>>>>>> synchronized (this) {
>>>>>>>>>>> if (this.stackTrace == null && // Immutable stack
>>>>>>>>>>> backtrace == null) // Test for out of
>>>>>>>>>>> protocol state
>>>>>>>>>>> return;
>>>>>>>>>>> + numStackTracesFilledIn++;
>>>>>>>>>>> this.stackTrace = defensiveCopy;
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> as that would seem to be another hole in the mechanism.
>>>>>>>>>>>
>>>>>>>>>>>> I want to vote again for the much more simple version
>>>>>>>>>>>> proposed in webrev 02:
>>>>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr20/8248476-
>>>>>>> NPE_fillInStackTrace-
>>>>>>>>>>> jdk15/02/
>>>>>>>>>>>
>>>>>>>>>>> I much prefer the latest version that recognises that only the
>>>>>>>>>>> original
>>>>>>>>>>> stack can be processed.
>>>>>>>>>>>
>>>>>>>>>>> In the test:
>>>>>>>>>>>
>>>>>>>>>>> + // This holds for explicitly crated NPEs, but also
>>>>>>>>>>> for implicilty
>>>>>>>>>>>
>>>>>>>>>>> Two typos: crated & implicilty
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> It's drawback is only that for this code:
>>>>>>>>>>>> ex = null;
>>>>>>>>>>>> ex.fillInStackTrace()
>>>>>>>>>>>> no message is created.
>>>>>>>>>>>>
>>>>>>>>>>>> I think this really is acceptable.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Remi, I didn't comment on this statement from a previous mail:
>>>>>>>>>>>>>> Hmm, Throwable.stackTrace is used for the stack trace at
>> some
>>>>>>> point.
>>>>>>>>>>>>> yes, it contains the Java stack trace, but if the Java stack
>>>>>>>>>>>>> trace is
>>>>> filled
>>>>>>>>> you
>>>>>>>>>>> don't
>>>>>>>>>>>>> compute any helpful message anyway.
>>>>>>>>>>>> The internal structure is no more deleted when the stack trace
>>>>>>>>>>>> is filled. So the message can be computed later, too.
>>>>>>>>>>>>
>>>>>>>>>>>> Best regards,
>>>>>>>>>>>> Goetz.
>>>>>>>>>>>>
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: forax at univ-mlv.fr <forax at univ-mlv.fr>
>>>>>>>>>>>>> Sent: Thursday, July 2, 2020 8:52 PM
>>>>>>>>>>>>> To: Alan Bateman <Alan.Bateman at oracle.com>
>>>>>>>>>>>>> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
>> Christoph
>>>>>>> Dreis
>>>>>>>>>>>>> <christoph.dreis at freenet.de>; hotspot-runtime-dev <hotspot-
>>>>>>> runtime-
>>>>>>>>>>>>> dev at openjdk.java.net>; David Holmes
>>>>> <david.holmes at oracle.com>
>>>>>>>>>>>>> Subject: Re: [15] RFR: 8248476: No helpful
>> NullPointerException
>>>>>>>>> message
>>>>>>>>>>>>> after calling fillInStackTrace
>>>>>>>>>>>>>
>>>>>>>>>>>>> yes,
>>>>>>>>>>>>> it's what i was saying,
>>>>>>>>>>>>> given that a NPE can be thrown very early, before VarHandle is
>>>>>>>>> initialized,
>>>>>>>>>>> i
>>>>>>>>>>>>> believe that declaring numStackTracesFilledIn volatile is the
>>>>>>>>>>>>> best
>>>>> way
>>>>>>> to
>>>>>>>>>>>>> tackle that.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Rémi
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----- Mail original -----
>>>>>>>>>>>>>> De: "Alan Bateman" <Alan.Bateman at oracle.com>
>>>>>>>>>>>>>> À: "Goetz Lindenmaier" <goetz.lindenmaier at sap.com>,
>>>>> "Christoph
>>>>>>>>>>> Dreis"
>>>>>>>>>>>>> <christoph.dreis at freenet.de>
>>>>>>>>>>>>>> Cc: "hotspot-runtime-dev" <hotspot-runtime-
>>>>>>> dev at openjdk.java.net>,
>>>>>>>>>>>>> "David Holmes" <david.holmes at oracle.com>, "Remi Forax"
>>>>>>>>>>>>>> <forax at univ-mlv.fr>
>>>>>>>>>>>>>> Envoyé: Jeudi 2 Juillet 2020 20:47:31
>>>>>>>>>>>>>> Objet: Re: [15] RFR: 8248476: No helpful
>> NullPointerException
>>>>>>> message
>>>>>>>>>>>>> after calling fillInStackTrace
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 02/07/2020 17:45, Lindenmaier, Goetz wrote:
>>>>>>>>>>>>>>> Hi Christoph,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I fixed the comment, thanks for pointing that out.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> One other thing is that NPE::getMessage reads
>>>>>>> numStackTracesFilledIn
>>>>>>>>>>>>>> without synchronization.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -Alan
More information about the hotspot-runtime-dev
mailing list