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

[Mario Torre]

Thank you for contributing!

Cheers,
Mario
On Mon, Oct 1, 2018 at 3:48 PM Laurent Bourgès
<bourges.laurent at gmail.com> wrote:
>
> 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



-- 
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH <https://www.redhat.com>
9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898