[15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Jul 14 13:48:26 UTC 2020
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?
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