[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 16:03]:
> > (i.e. where a PAV instance for that applet may not exist yet).
> 
> I see.
> 
> I'm ok with this patch then (after the nano/milli problem Omair
> spotted is fixed).
> 

Awesome, thanks for reviewing! Likewise for Omair, Andrew and Jiri --
thanks.

Cheers,
Deepak

> Regards,
> Denis.
> 
> ----- Original Message -----
> > * 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 
> > 
> > > 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.
> > > >