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

[Denis Lila]

> (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).

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.
> > >