RFR 8208303: Track JNI failures and fail tests

JC Beyler jcbeyler at google.com
Wed Aug 8 22:38:21 UTC 2018


Hi Alexey,

Thanks for the review. I still do have to send a webrev up because I would
need someone to sponsor it, test it, and push it :-) (I think Serguei said
he would do it or was doing it so we might have to wait for his return).

So here it is with the reviewed-by filled in:
http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.04/

But for reference here, I actually changed this to:

-      fprintf(stderr, "fill_native_frames: Exception in jni
NewWeakGlobalRef\n");
+      JNI_ENV_PTR(jni)->FatalError(
+          JNI_ENV_ARG2(jni, "Error in event_storage_add: Exception in jni
NewWeakGlobalRef"));

which is consistent with what we would want : fail in the native code if
there is a problem that is unexpected via FatalError.

Thanks for the review!
Jc





On Wed, Aug 8, 2018 at 2:43 PM Alex Menkov <alexey.menkov at oracle.com> wrote:

> Hi Jc,
>
> The fix looks good to me too.
>
> minor note:
>   564     if (JNI_ENV_PTR(jni)->ExceptionOccurred(JNI_ENV_ARG(jni))) {
>   565       fprintf(stderr, "fill_native_frames: Exception in jni
> NewWeakGlobalRef\n");
>   566     }
>
> I'd expect error message mention "event_storage_add" instead
> "fill_native_frames"
> No need to resent webrev.
>
> --alex
>
> On 08/08/2018 10:56, JC Beyler wrote:
> > Hi Serguei,
> >
> > Sounds good to me. If someone else could review it, I could add the
> > metadata to the webrev and, once testing (as you say) confirms nothing
> > is broken, we could push it in.
> >
> > If someone is motivated, that would be awesome and much appreciated!
> > Jc
> >
> > On Wed, Aug 8, 2018 at 12:08 AM serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com>> wrote:
> >
> >     Hi Jc,
> >
> >     This looks good to me especially because we discussed it a lot
> >     internally.
> >     The testing looks pretty good too.
> >     I submitted mach5 jobs for HepMonitor tests repeating 100 times and
> >     also normal tier1, tier2, hs-tier2-5.
> >     But it was not tested with the tiers 6 & 7 yet (most likely, need to
> >     make sure they run well too).
> >
> >     Thanks,
> >     Serguei
> >
> >
> >     On 8/6/18 08:27, JC Beyler wrote:
> >>     Hi all,
> >>
> >>     I updated the version and Serguei tested it for me so this change
> >>     should not change the stability of the tests.  This has the
> >>     advantage of not having to complicate the Java tests at all, while
> >>     adding the post-JNI call checks.
> >>
> >>     Webrev: http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.04/
> >>     <http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.04/>
> >>     Bug: https://bugs.openjdk.java.net/browse/JDK-8208303
> >>
> >>     Thanks all for your reviews!
> >>     Jc
> >>
> >>     On Fri, Jul 27, 2018 at 4:36 PM JC Beyler <jcbeyler at google.com
> >>     <mailto:jcbeyler at google.com>> wrote:
> >>
> >>         Hi all,
> >>
> >>         I did the new version that calls FatalError if JNI fails a
> >>         call. This has the advantage of not having to complicate the
> >>         Java tests at all, while adding the post-JNI call checks.
> >>
> >>         Webrev:
> >>         http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.03/
> >>         <http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.03/>
> >>         Bug: https://bugs.openjdk.java.net/browse/JDK-8208303
> >>
> >>         Thanks all!
> >>         Jc
> >>
> >>         On Thu, Jul 26, 2018 at 12:52 PM Chris Plummer
> >>         <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>>
> >>         wrote:
> >>
> >>             I'm pretty sure changes that only affect tests can be any
> >>             priority. But still, be a lot more cautious the closer we
> >>             get to release.
> >>
> >>             Chris
> >>
> >>             On 7/26/18 12:15 PM, Daniel D. Daugherty wrote:
> >>>             We entered RDP2 today (07.26). So only P1 and P2 bug
> >>>             fixes allowed.
> >>>
> >>>             Dan
> >>>
> >>>
> >>>             On 7/26/18 3:14 PM, serguei.spitsyn at oracle.com
> >>>             <mailto:serguei.spitsyn at oracle.com> wrote:
> >>>>             Yes, of course it has to be well tested before the push.
> >>>>             Does it make sense to plan it to push to 11 (after th
> >>>>             testing is done)?
> >>>>
> >>>>             Thanks,
> >>>>             Serguei
> >>>>
> >>>>
> >>>>             On 7/26/18 12:08, Daniel D. Daugherty wrote:
> >>>>>             Please make sure this fix is well tested in Mach5 prior
> >>>>>             to pushing.
> >>>>>             In particular, I'm focused on reducing the noise in
> >>>>>             Mach5 tier[1-3]
> >>>>>             so adding any new failures there will make me grumpy :-)
> >>>>>
> >>>>>             Dan
> >>>>>
> >>>>>
> >>>>>             On 7/26/18 3:03 PM, JC Beyler wrote:
> >>>>>>             Hi all,
> >>>>>>
> >>>>>>             With the FatalError idea, here is the webrev to
> >>>>>>             consider, note it no longer changes the tests. If a
> >>>>>>             JNI call fails, then we call FatalError.
> >>>>>>
> >>>>>>             Let me know what you think:
> >>>>>>
> >>>>>>             Webrev:
> >>>>>>             http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.01/
> <http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.01/>
> >>>>>>             Bug: https://bugs.openjdk.java.net/browse/JDK-8208303
> >>>>>>
> >>>>>>             Thanks!
> >>>>>>             Jc
> >>>>>>
> >>>>>>             On Thu, Jul 26, 2018 at 10:46 AM
> >>>>>>             serguei.spitsyn at oracle.com
> >>>>>>             <mailto:serguei.spitsyn at oracle.com>
> >>>>>>             <serguei.spitsyn at oracle.com
> >>>>>>             <mailto:serguei.spitsyn at oracle.com>> wrote:
> >>>>>>
> >>>>>>                 Hi Jc,
> >>>>>>
> >>>>>>                 Good idea.
> >>>>>>                 I was thinking about something like this.
> >>>>>>
> >>>>>>                 Thanks,
> >>>>>>                 Serguei
> >>>>>>
> >>>>>>
> >>>>>>                 On 7/26/18 10:40, JC Beyler wrote:
> >>>>>>>                 Hi Serguei,
> >>>>>>>
> >>>>>>>                 As I was looking at another test bug
> >>>>>>>                 (https://bugs.openjdk.java.net/browse/JDK-8191519
> );
> >>>>>>>                 the proposal for that bug is to have a JNI call
> >>>>>>>                 to FatalError to provoke a failure.
> >>>>>>>
> >>>>>>>                 If we went down that route, this webrev is
> >>>>>>>                 simpler, no? Instead of setting failure_status
> >>>>>>>                 and checking it later; just fail fatally and be
> >>>>>>>                 done with it, no? That way, the tests in Java
> >>>>>>>                 land don't have to be changed actually, no?
> >>>>>>>
> >>>>>>>                 What would we prefer for tests? Remember there
> >>>>>>>                 was a failure and test it later or fail fast via
> >>>>>>>                 JNI's FatalError?
> >>>>>>>
> >>>>>>>                 Thanks,
> >>>>>>>                 Jc
> >>>>>>>
> >>>>>>>
> >>>>>>>                 On Thu, Jul 26, 2018 at 10:04 AM
> >>>>>>>                 serguei.spitsyn at oracle.com
> >>>>>>>                 <mailto:serguei.spitsyn at oracle.com>
> >>>>>>>                 <serguei.spitsyn at oracle.com
> >>>>>>>                 <mailto:serguei.spitsyn at oracle.com>> wrote:
> >>>>>>>
> >>>>>>>                     Hi Jc,
> >>>>>>>
> >>>>>>>                     It looks good to me.
> >>>>>>>
> >>>>>>>                     Thanks,
> >>>>>>>                     Serguei
> >>>>>>>
> >>>>>>>
> >>>>>>>                     On 7/26/18 09:58, JC Beyler wrote:
> >>>>>>>>                     Hi all,
> >>>>>>>>
> >>>>>>>>                     The tests in the HeapMonitor subsystem has a
> >>>>>>>>                     lot of JNI calls. There is a need for
> >>>>>>>>                     verification and testing if anything in the
> >>>>>>>>                     JNI subsystem failed unexpectedly.
> >>>>>>>>
> >>>>>>>>                     Here is a webrev that tracks if a JNI call
> >>>>>>>>                     does fail and the tests will fail if any JNI
> >>>>>>>>                     call does fail.
> >>>>>>>>
> >>>>>>>>                     Could I have a few reviews please for:
> >>>>>>>>                     Webrev:
> >>>>>>>>
> http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.00/
> >>>>>>>>                     <
> http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.00/>
> >>>>>>>>                     Bug:
> >>>>>>>>
> https://bugs.openjdk.java.net/browse/JDK-8208303
> >>>>>>>>
> >>>>>>>>                     Thanks,
> >>>>>>>>                     Jc
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>                 --
> >>>>>>>
> >>>>>>>                 Thanks,
> >>>>>>>                 Jc
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>             --
> >>>>>>
> >>>>>>             Thanks,
> >>>>>>             Jc
> >>>>>
> >>>>
> >>>
> >>
> >>
> >>
> >>         --
> >>
> >>         Thanks,
> >>         Jc
> >>
> >>
> >>
> >>     --
> >>
> >>     Thanks,
> >>     Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180808/1b617522/attachment-0001.html>


More information about the serviceability-dev mailing list