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