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

[Denis Lila]

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

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

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

----- Original Message -----

> 
> I'm still a bit dubious about this class as a whole and this change
> needs heavy testing before inclusion. Can someone try this out? I have
> no idea how this stuff actually is run and tested sufficiently.
> 
> 
> [Apologies in advance for a few whitespace issues in the patch.
> Clearly
> someone has previously regressed on our identation cleanup and emacs
> is auto-cleaning it up. I can easily drop these from the committed
> version using -w if required].
> 
> ChangeLog:
> 
> 2010-04-14 Andrew John Hughes <ahughes at redhat.com>
> 
> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java,
> (PluginAppletPanelFactory.createPanel(PluginStreamHandler,
> int,long,int,int,URL,Hashtable)): Remove duplication of wait
> for panel.isAlive().
> (PluginAppletViewer.panelLock): New lock used to track panel
> creation.
> (PluginAppletViewer.panelLive): Condition queue for panel creation.
> (PluginAppletViewer.appletsLock): New lock used to track additions
> to the applets map.
> (PluginAppletViewer.appletAdded): Condition queue for applet addition.
> (PluginAppletViewer.statusLock): New lock for status changes.
> (PluginAppletViewer.initComplete): Condition queue for initialisation
> completion.
> (PluginAppletViewer.framePanel(int,long,NetxPanel)):
> Replace synchronized block with use of appletsLock and notification
> on appletAdded condition queue.
> (AppletEventListener.appletStateChanged(AppletEvent)): Signal the
> panelLive condition queue that the panel is live.
> (PluginAppletViewer.handleMessage(int,int,String)): Wait on
> appletAdded
> condition queue for applet to be added to the applets map.
> (PluginAppletViewer.updateStatus(Int,PAV_INIT_STATUS)): Signal when a
> status change occurs using the initComplete condition queue.
> (PluginAppletViewer.waitForAppletInit(NetxPanel)): Wait on the
> panelLive
> condition queue until the panel is created.
> (PluginAppletViewer.handleMessage(int,String)): Wait on the
> initComplete
> condition queue until initialisation is complete. Wait on the
> panelLive
> signal until panel is created.
> (waitTillTimeout(ReentrantLock,Condition,long)): Convert to use
> ReentrantLock and Condition. Add assertion to check the lock is held.
> Avoid conversion between milliseconds and nanoseconds.
> 
> --
> 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