[15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace
forax at univ-mlv.fr
forax at univ-mlv.fr
Tue Jul 14 17:30:34 UTC 2020
----- Mail original -----
> De: "Goetz Lindenmaier" <goetz.lindenmaier at sap.com>
> À: "David Holmes" <david.holmes at oracle.com>, "Remi Forax" <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>
> Envoyé: Mardi 14 Juillet 2020 15:48:26
> Objet: RE: [15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace
> 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/
> 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 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?
Hi Goetz,
this is a review for the v07,
the static final fields should be in uppercase to make the code more readable,
you need to use new String("1") because "1" may be a valid string, i also think that "1" and "2" are not explicit enough, using something like "MUST_COMPUTE_EXTENTED_NPE_MESSAGE" seems better IMO.
i don't think you need to declare extendedMessage volatile, it is only accessed inside a synchronized block on this.
in getMessage, you can use a early return to simplify the code shape
synchronized(this) {
if (extendedMessage == mustComputeExtendedNPEMessage) {
// Only the original stack trace was filled in. Message will
// compute correctly.
return extendedMessage = getExtendedNPEMessage(); // <-- HERE
}
...
>
> Best regards,
> Goetz.
regards,
Rémi
>
>
>
>
>> -----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