[icedtea-web] RFC: Switch to explicit locks and condition queues in PAV
Deepak Bhole
dbhole at redhat.com
Fri Apr 15 07:41:12 PDT 2011
* Dr Andrew John Hughes <ahughes at redhat.com> [2011-04-14 18:44]:
> 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.
>
> 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.
>
I don't know if we should be rewriting it. It took us a very long time
to stabilize it (and as bad as the code is, it requires very little
maintenance considering how much it does. Even the synch. issues Denis
found have never manifested themselves afaik). This is one of the
reasons I imported the class from the old plugin into the new one.
That said, I strongly feel that it needs to be re-factored and made
clean though, and I am looking into that now.
Cheers,
Deepak
> 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. At present,
> I have no overview of what this class is doing and it's far too big.
>
> [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
> diff -r b4db997469a2 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu Apr 14 21:41:32 2011 +0100
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu Apr 14 22:53:10 2011 +0100
> @@ -97,8 +97,13 @@
> import java.util.Iterator;
> import java.util.Map;
> import java.util.Vector;
> +
> import java.util.concurrent.ConcurrentHashMap;
> import java.util.concurrent.ConcurrentMap;
> +import java.util.concurrent.TimeUnit;
> +
> +import java.util.concurrent.locks.Condition;
> +import java.util.concurrent.locks.ReentrantLock;
>
> import javax.swing.SwingUtilities;
>
> @@ -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();
> @@ -263,12 +250,17 @@
> * The panel in which the applet is being displayed.
> */
> private NetxPanel panel;
> -
> + static final ReentrantLock panelLock = new ReentrantLock();
> + // CONDITION PREDICATE: panel.isAlive()
> + static final Condition panelLive = panelLock.newCondition();
> private int identifier;
>
> // Instance identifier -> PluginAppletViewer object.
> private static ConcurrentMap<Integer, PluginAppletViewer> applets =
> new ConcurrentHashMap<Integer, PluginAppletViewer>();
> + private static final ReentrantLock appletsLock = new ReentrantLock();
> + // CONDITION PREDICATE: !applets.containsKey(identifier)
> + private static final Condition appletAdded = appletsLock.newCondition();
>
> private static PluginStreamHandler streamhandler;
>
> @@ -276,6 +268,9 @@
>
> private static ConcurrentMap<Integer, PAV_INIT_STATUS> status =
> new ConcurrentHashMap<Integer, PAV_INIT_STATUS>();
> + private static final ReentrantLock statusLock = new ReentrantLock();
> + // CONDITION PREDICATE: !status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE)
> + private static final Condition initComplete = statusLock.newCondition();
>
> private WindowListener windowEventListener = null;
> private AppletEventListener appletEventListener = null;
> @@ -309,11 +304,10 @@
> appletFrame.appletEventListener = new AppletEventListener(appletFrame, appletFrame);
> panel.addAppletListener(appletFrame.appletEventListener);
>
> -
> - synchronized(applets) {
> - applets.put(identifier, appletFrame);
> - applets.notifyAll();
> - }
> + appletsLock.lock();
> + applets.put(identifier, appletFrame);
> + appletAdded.signalAll();
> + appletsLock.unlock();
>
> PluginDebug.debug(panel, " framed");
> }
> @@ -364,9 +358,9 @@
> public void appletStateChanged(AppletEvent evt) {
> AppletPanel src = (AppletPanel) evt.getSource();
>
> - synchronized (src) {
> - src.notifyAll();
> - }
> + panelLock.lock();
> + panelLive.signalAll();
> + panelLock.unlock();
>
> switch (evt.getID()) {
> case AppletPanel.APPLET_RESIZE: {
> @@ -460,12 +454,17 @@
> new URL(documentBase));
>
> long maxTimeToSleep = APPLET_TIMEOUT;
> - synchronized(applets) {
> + appletsLock.lock();
> + try {
> while (!applets.containsKey(identifier) &&
> maxTimeToSleep > 0) { // Map is populated only by reFrame
> - maxTimeToSleep -= waitTillTimeout(applets, maxTimeToSleep);
> + maxTimeToSleep -= waitTillTimeout(appletsLock, appletAdded,
> + maxTimeToSleep);
> }
> }
> + finally {
> + appletsLock.unlock();
> + }
>
> // If wait exceeded maxWait, we timed out. Throw an exception
> if (maxTimeToSleep <= 0)
> @@ -552,11 +551,11 @@
> }
>
> // Else set to given status
> -
> - synchronized(status) {
> - status.put(identifier, newStatus);
> - status.notifyAll();
> - }
> +
> + statusLock.lock();
> + status.put(identifier, newStatus);
> + initComplete.signalAll();
> + statusLock.unlock();
>
> return prev;
> }
> @@ -610,7 +609,7 @@
>
> /**
> * Function to block until applet initialization is complete.
> - *
> + *
> * This function will return if the wait is longer than {@link #APPLET_TIMEOUT}
> *
> * @param panel the instance to wait for.
> @@ -620,14 +619,18 @@
> // Wait till initialization finishes
> long maxTimeToSleep = APPLET_TIMEOUT;
>
> - synchronized(panel) {
> + panelLock.lock();
> + try {
> while (panel.getApplet() == null &&
> panel.isAlive() &&
> maxTimeToSleep > 0) {
> PluginDebug.debug("Waiting for applet panel ", panel, " to initialize...");
> - maxTimeToSleep -= waitTillTimeout(panel, maxTimeToSleep);
> + maxTimeToSleep -= waitTillTimeout(panelLock, panelLive, maxTimeToSleep);
> }
> }
> + finally {
> + panelLock.unlock();
> + }
>
> PluginDebug.debug("Applet panel ", panel, " initialized");
> }
> @@ -637,12 +640,17 @@
>
> // Wait for panel to come alive
> long maxTimeToSleep = APPLET_TIMEOUT;
> - synchronized(status) {
> + statusLock.lock();
> + try {
> while (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> maxTimeToSleep > 0) {
> - maxTimeToSleep -= waitTillTimeout(status, maxTimeToSleep);
> + maxTimeToSleep -= waitTillTimeout(statusLock, initComplete,
> + maxTimeToSleep);
> }
> }
> + finally {
> + statusLock.unlock();
> + }
>
> // 0 => width, 1=> width_value, 2 => height, 3=> height_value
> String[] dimMsg = message.split(" ");
> @@ -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
> @@ -1370,7 +1375,7 @@
>
> /**
> * Decodes the string (converts html escapes into proper characters)
> - *
> + *
> * @param toDecode The string to decode
> * @return The decoded string
> */
> @@ -1385,7 +1390,7 @@
>
> return toDecode;
> }
> -
> +
> /**
> * System parameters.
> */
> @@ -1571,7 +1576,7 @@
> public void run() {
> ClassLoader cl = p.applet.getClass().getClassLoader();
>
> - // Since we want to deal with JNLPClassLoader, extract it if this
> + // Since we want to deal with JNLPClassLoader, extract it if this
> // is a codebase loader
> if (cl instanceof JNLPClassLoader.CodeBaseClassLoader)
> cl = ((JNLPClassLoader.CodeBaseClassLoader) cl).getParentJNLPClassLoader();
> @@ -2060,34 +2065,35 @@
> }
>
> /**
> - * Waits on a given object until timeout.
> - *
> + * Waits on a given condition queue until timeout.
> + *
> * <b>This function assumes that the monitor lock has already been
> * acquired by the caller.</b>
> - *
> - * If the given object is null, this function returns immediately.
> - *
> - * @param obj The object to wait on
> + *
> + * If the given lock is null, this function returns immediately.
> + *
> + * @param lock the lock that must be held when this method is called.
> + * @param cond the condition queue on which to wait for notifications.
> * @param timeout The maximum time to wait (nanoseconds)
> * @return Approximate time spent sleeping (not guaranteed to be perfect)
> */
> - public static long waitTillTimeout(Object obj, long timeout) {
> + public static long waitTillTimeout(ReentrantLock lock, Condition cond,
> + long timeout) {
>
> // Can't wait on null. Return 0 indicating no wait happened.
> - if (obj == null)
> + if (lock == null)
> return 0;
> -
> +
> + assert lock.isHeldByCurrentThread();
> +
> // Record when we started sleeping
> long sleepStart = 0L;
>
> - // Convert timeout to ms since wait() takes ms
> - timeout = timeout >= 1000000 ? timeout/1000000 : 1;
> -
> try {
> sleepStart = System.nanoTime();
> - obj.wait(timeout);
> + cond.await(timeout, TimeUnit.NANOSECONDS);
> } catch (InterruptedException ie) {} // Discarded, time to return
> -
> +
> // Return the difference
> return System.nanoTime() - sleepStart;
> }
More information about the distro-pkg-dev
mailing list