[15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Jul 14 14:53:40 UTC 2020
Hi David,
Sorry, so far I only responded to the "correction" mail.
Yes, you describe it exactly as I see it.
To the user it should not be visible that the message is computed
lazy. It should just feel like any other message. The lazy computation
is only meant to improve performance of discarded exceptions.
This is just the same as with the backtrace data structure.
No one ever sees this on user side, the backtrace -> stacktrace
conversion is done internally on demand.
The actual NPE message implementation does not honor this
principle always, but that is how I would liked to have it.
If we go with the latest webrevs, it's according to this principle.
Best regards,
Goetz.
> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Tuesday, July 14, 2020 4:11 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,
>
> 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();
> }
>
> 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