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

[Deepak Bhole]

* 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.
> > > 
> > 
> > Please see attached new patch. After leaving the office I realized that
> > there is a potential race condition in the old patch with locking the
> > object inside the wait function. Patch updated to do the locking
> > outside (and function comment updated to clearly state that).
> > 
> 
> Can you elaborate?  If it's the same thread acquiring the lock in both
> cases, then it should be fine as they are re-entrant i.e. if the function
> attempts to lock it and the thread already holds the lock, it's a no-op.
> 

Before, the locking was inside the function, like so:

while !<check contition on OBJ>
    waitTillTimeout(OBJ, timeout)
...
...


function ... waitTillTimeout(OBJ, timeout) {
    
    lock (OBJ)
    OBJ.wait()
    ...
}

The problem was that condition on OBJ could've been false at check, but then
become true before waitTillTimeout() was called. This would result in an
incorrect wait until timeout. By moving the lock to before condition checking,
the code guarantees that waiting is done only if needed. 

As I wrote this, I realized that in order for the above to work, we would have
to lock the hashmap on update too since CHM's locking works differently and may
not lock the full table. I will post a separate patch in a bit with the change.
In the mean time, I have replied to other concerns below.

> > Cheers,
> > Deepak
> 
> Comments inline.  As Jiri said, a ChangeLog would be good too.
> 

Oops, sorry I should've mentioned, the ChangeLog is the same as before
(only the locking area changed). Here it is:

2011-04-13  Deepak Bhole <dbhole at redhat.com>

    * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java (createPanel):
    use Object.wait() to wait, rather than pariodic sleep.
    (framePanel): Notify threads waiting on the applets map instance.
    (appletStateChanged): Notify all threads waiting on the panel that just
    changed state.
    (handleMessage): Use the new waitTillTimeout function to wait, rather than
    periodically waking up.
    (updateStatus): Notify all threads waiting on status map.
    (waitForAppletInit): Use the new waitTillTimeout function to wait, rather
    than periodically waking up.
    (waitTillTimeout): New function. For a given non-null object, waits until
    the specified timeout, or, if an interrupt was thrown during wait, returns
    immediately.

> > -                // just wait
> > +        // Wait implemented the long way so that
> > +        // PluginAppletViewer.waitTillTimeout() needn't be exposed.
> > +        long maxTimeToSleep = PluginAppletViewer.APPLET_TIMEOUT;
> > +        
> > +        synchronized(panel) {
> > +            while (!panel.isAlive() && maxTimeToSleep > 0) {
> > +                long sleepStart = System.currentTimeMillis();
> > +
> > +                try {
> > +                    panel.wait(maxTimeToSleep);
> > +                } catch (InterruptedException e) {} // Just loop back
> > +
> > +                maxTimeToSleep -= (System.currentTimeMillis() - sleepStart);
> 
> So this one matches src.wait() below?  It's a little unclear due to the differing
> variable names.
> 
> Is there a reason why this doesn't use waitTillTimeout?
> 

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.

> >              }
> >          }
> >  
> > @@ -307,6 +310,10 @@
> >          panel.addAppletListener(appletFrame.appletEventListener);
> >  
> >          applets.put(identifier, appletFrame);
> > +        
> > +        synchronized(applets) {
> > +            applets.notifyAll();
> > +        }
> >  
> >          PluginDebug.debug(panel, " framed");
> >      }
> 
> This change is correct, but I'm a bit dubious about having one set of
> locks in the CHM for the put and our own lock on applets here.  Maybe
> revert back to HashMaps and handle all locking in one place?
>

We would then have to lock on all gets too. CHMs locking is more
efficient as it locks on only parts of the table. That is why I chose to
do locking only around code that needs it (the API requires wait and
notifyall to be calls from threads that own the monitor lock).

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

> >                  }
> >              }
> >  
> > @@ -686,17 +693,21 @@
> >              // FIXME: how do we determine what security context this
> >              // object should belong to?
> >              Object o;
> > -
> > -            // Wait for panel to come alive
> > -            int maxWait = APPLET_TIMEOUT; // wait for panel to come alive
> > -            int wait = 0;
> > -            while ((panel == null) || (!panel.isAlive() && wait < maxWait)) {
> > +            
> > +            // First, wait for panel to instantiate
> > +            int waited = 0;
> > +            while (panel == null && waited < APPLET_TIMEOUT) {
> >                  try {
> >                      Thread.sleep(50);
> > -                    wait += 50;
> > -                } catch (InterruptedException ie) {
> > -                    // just wait
> > -                }
> > +                    waited += 50;
> > +                } catch (InterruptedException ie) {} // discard, loop back
> > +            }
> > +
> > +            // Next, wait for panel to come alive
> > +            long maxTimeToSleep = APPLET_TIMEOUT;
> > +            synchronized(panel) {
> > +                while (!panel.isAlive())
> > +                    maxTimeToSleep -= waitTillTimeout(panel, maxTimeToSleep);
> 
> I presume panel is global to this instance?
> 
> Maybe we should introduce something else to provide the conditional wait so you
> don't need this split between the null/sleep(50) loop and the initialising/notification loop.
> 

Yeah I wasn't too happy about having to keep the above. I thought about
introducing an additional lock for the object, but the truth is that the
sleep will happen for a very short time, if it ever does. Furthermore,
the above would get called only for liveconnect based applets, so it
didn't seem worth it. I can still do it if you prefer though.

> 
> Would have been nice to have closures for this.  Then you could pass the condition
> to this method, and have the whole block handled here. :-)
> 

Only 20 more months if all goes well! :)

Cheers,
Deepak

> >  }
> 
> 
> -- 
> Andrew :)
> 
> Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
> 
> Support Free Java!
> Contribute to GNU Classpath and IcedTea
> http://www.gnu.org/software/classpath
> http://icedtea.classpath.org
> PGP Key: F5862A37 (https://keys.indymedia.org/)
> Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37