[rfc] [icedtea-web] Fix EDT hanging on OpenJDK 11

[Laurent Bourgès]

Thank you for reviews,
Laurent

Le lun. 1 oct. 2018 à 15:16, Mario Torre <neugens at redhat.com> a écrit :

> Hi Laurent,
>
> Yes, I approve.
>
> I think we need to rework the threading aspect though.
>
> Jiri, could you please push the patch for Laurent?
>
> Cheers,
> Mario
>
> On Mon, Oct 1, 2018 at 12:13 PM Laurent Bourgès
> <bourges.laurent at gmail.com> wrote:
> >
> > Mario,
> > Do you approve the latest webrev, as it is ?
> > Let's move forward, after this patch integrated.
> > Laurent
> >
> > Le lun. 1 oct. 2018 à 11:17, Jiri Vanek <jvanek at redhat.com> a écrit :
> >>
> >> ...
> >> >
> >> >      >     3. It seems that methods that use "observable" are
> synchronized.
> >> >      >     However, the EDT will use "observable" and it's not
> synchronized. Is
> >> >      >     this safe?
> >> >      >
> >> >      >
> >> >      > Ok, fixed:
> >> >      >                 synchronized(observable) {
> >> >      >                     if (observable.hasChanged() ||
> >> >      > (Boolean.TRUE.equals(force))) {
> >> >      >                         observable.notifyObservers(force);
> >> >      >                     }
> >> >      >                 }
> >> >
> >> >     Right, but I don't think this addresses the issue, one is
> synchronized
> >> >     on the observable, the other methods are on the JavaConsole, so we
> >> >     should probably pick one.
> >> >
> >> >
> >> > Sorry, I did not catch the problem.
> >> > My fix is very minimal and this patch does not fix synchronization
> issues already present in ITW.
> >> > I tested javaws with the console opened and did not notice any
> problem.
> >> >
> >> >
> >> >     By going through the code though, it seems like we could just
> ensure
> >> >     that updateModel() always run in the EDT and remove all other
> >> >     synchronised keywords from the methods, the only user of this
> method
> >> >     which can execute outside the EDT is addMessage, but otherwise
> all other
> >> >     consumers are executing in the EDT.
> >> >
> >> >
> >> > Could we postpone this point later as I did not want to rewrite too
> much the JavaConsole ?
> >> > You propose to remove all 'synchronized' keywords in JavaConsole, I
> am pretty sure the
> >>
> >> I would strongly advice against removing the synchronizations. The
> logging bottleneck is lazily
> >> initiated singleton, and there were many troubles without it. Maybe
> with plugin gone, the troubles
> >> will decrease a lot, But i doubt.
> >>
> >> On contrary:
> >>
> >>  >on the observable, the other methods are on the JavaConsole, so we
> >>  >     should probably pick one.
> >>
> >> Sounds like straightforward fix. Anyway, this is on your judgement.
> >>
> >> > synchronization overhead does not hurt here...
> >> > If you could propose an alternative, I am OK to integrate it.
> >>
> >> Please, as another changeset (if any).
> >>
> >>
> >>
> >> Thanx!
> >>    J.
>
>
>
> --
> Mario Torre
> Associate Manager, Software Engineering
> Red Hat GmbH <https://www.redhat.com>
> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20181001/ed5c4e60/attachment.html>