[15] RFR: 8248476: No helpful NullPointerException message after calling fillInStackTrace

[Lindenmaier, Goetz]

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.

> 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 ��

> 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. 

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