[15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Jul 8 09:23:53 UTC 2020
Hi,
Is this good to be pushed now? I would like to push it
before RDP2 of jdk15, which is next week.
@David
> Sorry but that consistency argument is a huge stretch in the case of the
> helpful NPE message because the original message is empty!
No, it is not empty. It is computed lazy, but this is not visible to the
user. Especially, if I implement what you propose, the user can first
see the message, and then suddenly it is gone. This is really unexpected!
> This is only
> about helpful NPE message and you can trivially disable it for this case.
It's not hard to do it for all the exceptions, either. The counter
would have to be moved to Throwable, and all exceptions that
get a message from the runtime would have to be marked as such.
Then setStackTrace in throwable would just reset the message.
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