[15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace
David Holmes
david.holmes at oracle.com
Fri Jul 3 22:08:33 UTC 2020
On 3/07/2020 7:01 pm, Lindenmaier, Goetz wrote:
> Hi David,
>
>> 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.
> That is true. This is because it is complete nonsense to call setStackTrace() on the
> exception thrown by the runtime. If someone does so, it's his
> problem.
> We would have to fix this for all the exceptions thrown by the runtime
> that give a message. Else it is not consistent.
Sorry but that consistency argument is a huge stretch in the case of the
helpful NPE message because the original message is empty! This is only
about helpful NPE message and you can trivially disable it for this case.
> 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