[PATCH] JMC-6252 Method profiling rules loading forever

[Henrik Dafgård]

No worries, that's what the review process is there for! I think this looks
good and is ready to ship.


Regards,
Henrik Dafgård


On Wed, 29 May 2019 at 15:47, Kangcheng Xu <kxu at redhat.com> wrote:

> Hi Henrik,
>
> Thanks for the suggestions.
>
> On Wed, May 29, 2019 at 9:07 AM Henrik Dafgård <hdafgard at gmail.com> wrote:
> >
> > Hi,
> >
> > The patch looks great and solves a long-standing and annoying issue!
> >
> > However, since the check for whether or not the browser widget was
> disposed has been removed there will be exceptions thrown if the user
> navigates away while updates are being applied. This could be solved by
> e.g. checking whether or not the browser is disposed inside of the catch
> clause in the cmdExecRunnable lambda.
>
> The cmdExecRunnabe lambda has been updated to include a check for
> browser.isDisposed(). Sorry for making such an obvious mistake.
>
> > We might also consider changing the inner catch clause to no longer
> catch all exceptions (or at least avoid catching any RuntimeException
> thrown).
>
> The original code catches generic exceptions, so I didn't change this
> is. However, I do agree with you, the inner catch-clause now only
> catches IOException.
>
> >
> >
> > Regards,
> > Henrik Dafgård
>
> Thank you! Please see the updated patch attached.
>
> Arvin Kangcheng Xu
>
> >
> > On Tue, 28 May 2019 at 22:03, Kangcheng Xu <kxu at redhat.com> wrote:
> >>
> >> Hello,
> >>
> >> This patch fixes JMC-6252 Method Profiling rule appears to be taking
> >> forever to evaluate in Ubuntu 18.04 [0]. (Please note this issue is
> >> general and appears on all platforms.)
> >>
> >> This patch addresses two problems found in the source code:
> >> 1. The order of JavaScript command execution to update HTML-based UI
> >> could be stochastic if fired before DOM loaded. Commands executed as
> >> ProgressListener callbacks could be out-of-order. The solution is to
> >> store commands in a queue and execute in a FIFO fashion after DOM is
> >> loaded.
> >> 2. resultEventQueue.isEmpty() and resultEventQueue.add() together is
> >> not an atomic operation and could be interrupted in rare cases, which
> >> leads to resultEventQueue not being completely consumed.
> >>
> >> Please see the attached patch. Any thoughts on this fix? Thanks very
> much!
> >>
> >> Arvin Kangcheng Xu
> >>
> >> [0]
> >> https://bugs.openjdk.java.net/browse/JMC-6252
>