[icedtea-web] RFC: Patch to use wait() when waiting for conditions to become true

[Deepak Bhole]

* Denis Lila <dlila at redhat.com> [2011-04-14 14:47]:
> > I mentioned it in the comments, it is so that
> > PluginAppletViewer.waitTillTimeout() could remain private (the above
> > is in a
> > separate class). I intend to re-factor a lot of that code (already
> > started
> > working on it yesterday) that will address it there. In the mean time,
> > I
> > saw no harm in duplicating a few lines of code.
> 
> Will you make the factory a static inner class in PAV? I think having
> two separate classes in the same file is pretty ugly.
>

Yep.
 
> > > > - } catch (InterruptedException ie) {
> > > > - // just wait
> > > > + int maxTimeToSleep = APPLET_TIMEOUT;
> > > > + synchronized(status) {
> > > > + while
> > > > (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> > > > + maxTimeToSleep > 0) {
> > > > + maxTimeToSleep -= waitTillTimeout(status, maxTimeToSleep);
> > >
> > > Is status guaranteed to contain a mapping for identifier?
> > >
> > 
> > Yes, it is.
> 
> I never liked the existence of this "status" map. We need "applets"
> because we have static methods that have to have access to all PAV objects.
> status, on the other hand is just a way to associate some data
> with each PAV instance, but that's exactly what members are for.
> I think statuses should eventually become members, and updateStatus should
> stop being static (though not necessarily in this changeset).
> 

Not sure what you mean.. status maps are used by static methods as well.
They are used to handle cases where a window is closed during
initialization (i.e. where a PAV instance for that applet may not exist
yet). 

> One more thing: I think it might be better to introduce 2-3
> lock objects just for use with .wait and .notify. I don't think
> it's a very good idea to lock on status and applets because we
> don't know what kind of internal synchronization they use.

Regardless of what the internal synchronization is, the worst case I can
think of is unnecessary wake ups on notify (internal or in PAV), which I
don't think are serious enough to be creating multiple lock only objects
for.

Cheers,
Deepak

> It's generally safer to lock on private objects that only we
> have access to (and it also might reduce contention).
> 
> Other than these minor things, I can't find anything wrong
> with the patch (that is, your new patch, with nano time).
> 
> Regards,
> Denis.
> 
> ----- Original Message -----
> > * Dr Andrew John Hughes <ahughes at redhat.com> [2011-04-14 07:41]:
> > > On 01:56 Thu 14 Apr , Deepak Bhole wrote:
> > > > * Deepak Bhole <dbhole at redhat.com> [2011-04-13 20:48]:
> > > > > Hi,
> > > > >
> > > > > Andrew mentioned in one of the previous threads that the current
> > > > > plugin
> > > > > code waits in an inefficient manner. It wakes up periodically to
> > > > > check for conditions, and goes back to sleep, and keeps doing
> > > > > that
> > > > > cyclically.
> > > > >
> > > > > The attached patch uses a new function that uses object.wait for
> > > > > the object of interest.
> >