[icedtea-web] RFC: Switch to explicit locks and condition queues in PAV

[Dr Andrew John Hughes]

On 10:09 Fri 15 Apr     , Denis Lila wrote:
> > This is the change I mentioned in the previous discussion. It replaces
> > the use of CHM monitors with explicit locks and condition queues. It
> > also
> > simplifies (I hope) the panel lock case by using an explicit lock and
> > condition queue rather than a mix of the panel object and sleep.
> A few comments:
> 
> > @@ -139,25 +144,7 @@
> >          // Start the applet
> >          initEventQueue(panel);
> >  
> > -        // Wait for panel to come alive
> > -        // Wait implemented the long way so that
> > -        // PluginAppletViewer.waitTillTimeout() needn't be exposed.
> > -        long maxTimeToSleep = PluginAppletViewer.APPLET_TIMEOUT/1000000; // ns -> ms
> > -
> > -        synchronized(panel) {
> > -            while (!panel.isAlive() && maxTimeToSleep > 0) {
> > -                long sleepStart = System.nanoTime();
> > -
> > -                try {
> > -                    panel.wait(maxTimeToSleep);
> > -                } catch (InterruptedException e) {} // Just loop back
> > -
> > -                maxTimeToSleep -= (System.nanoTime() - sleepStart)/1000000; // ns -> ms
> > -            }
> > -        }
> > -
> >          // Wait for the panel to initialize
> > -        // (happens in a separate thread)
> >          PluginAppletViewer.waitForAppletInit(panel);
> >  
> >          Applet a = panel.getApplet();
> 
> The code above wasn't replaced by anything. Was this intentional?
> 

Yes, I think I mentioned it in the ChangeLog.  The code is identical to
PluginAppletViewer.waitForAppletInit(panel) i.e. we do it twice.

> > @@ -691,21 +699,18 @@
> >              // FIXME: how do we determine what security context this
> >              // object should belong to?
> >              Object o;
> > -            
> > +
> >              // First, wait for panel to instantiate
> > -            int waited = 0;
> > -            while (panel == null && waited < APPLET_TIMEOUT) {
> > -                try {
> > -                    Thread.sleep(50);
> > -                    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);
> > +            panelLock.lock();
> > +            try {
> > +                while (panel == null || !panel.isAlive())
> > +                    maxTimeToSleep -= waitTillTimeout(panelLock, panelLive,
> > +                                                      maxTimeToSleep);
> > +            }
> > +            finally {
> > +                panelLock.unlock();
> >              }
> >  
> >              // Wait for the panel to initialize
> 
> 
> I like this.
> 
> I should mention that the new code is not equivalent to the old.
> Now we're using a static panelLock while before we were using a different
> lock for each panel (i.e. the panel itself). This doesn't seem to be a
> problem, but it's worth noting.

Yeah this is my concern about applying this.  The replacement of the two map locks
is trivial.  This is kind of messy, but works AFAICS.

> 
> > Long term this class needs a ground-up redesign. We're patching holes
> > here when this needs to go back to the drawing board IMHO.
> 
> Won't that reintroduce bugs we've already fixed?

Maybe, but you have to trade that off sometimes for long-term maintainability.
When it comes to concurrency, it needs to be designed in from the start not
patched up on a case-by-case basis.


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