[icedtea-web] RFC: Patch to use wait() when waiting for conditions to become true
Dr Andrew John Hughes
ahughes at redhat.com
Thu Apr 14 04:41:00 PDT 2011
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.
> Cheers,
> Deepak
Comments inline. As Jiri said, a ChangeLog would be good too.
> diff -r 2ef65ff10619 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Wed Apr 13 19:17:21 2011 -0400
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu Apr 14 01:54:04 2011 -0400
> @@ -91,12 +91,10 @@
> import java.security.PrivilegedAction;
> import java.security.PrivilegedActionException;
> import java.security.PrivilegedExceptionAction;
> -import java.util.ArrayList;
> import java.util.Enumeration;
> import java.util.HashMap;
> import java.util.Hashtable;
> import java.util.Iterator;
> -import java.util.List;
> import java.util.Map;
> import java.util.Vector;
> import java.util.concurrent.ConcurrentHashMap;
> @@ -142,14 +140,19 @@
> initEventQueue(panel);
>
> // Wait for panel to come alive
> - int maxWait = PluginAppletViewer.APPLET_TIMEOUT; // wait for panel to come alive
> - int wait = 0;
> - while (!panel.isAlive() && wait < maxWait) {
> - try {
> - Thread.sleep(50);
> - wait += 50;
> - } catch (InterruptedException ie) {
> - // 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?
> }
> }
>
> @@ -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?
> @@ -357,6 +364,10 @@
> public void appletStateChanged(AppletEvent evt) {
> AppletPanel src = (AppletPanel) evt.getSource();
>
> + synchronized (src) {
> + src.notifyAll();
> + }
> +
> switch (evt.getID()) {
> case AppletPanel.APPLET_RESIZE: {
> if (src != null) {
> @@ -448,21 +459,16 @@
> new StringReader(tag),
> new URL(documentBase));
>
> - int maxWait = APPLET_TIMEOUT; // wait for applet to fully load
> - int wait = 0;
> - while (!applets.containsKey(identifier) && // Map is populated only by reFrame
> - (wait < maxWait)) {
> -
> - try {
> - Thread.sleep(50);
> - wait += 50;
> - } catch (InterruptedException ie) {
> - // just wait
> + long maxTimeToSleep = APPLET_TIMEOUT;
> + synchronized(applets) {
> + while (!applets.containsKey(identifier) &&
> + maxTimeToSleep > 0) { // Map is populated only by reFrame
> + maxTimeToSleep -= waitTillTimeout(applets, maxTimeToSleep);
> }
> }
>
> // If wait exceeded maxWait, we timed out. Throw an exception
> - if (wait >= maxWait)
> + if (maxTimeToSleep <= 0)
> throw new Exception("Applet initialization timeout");
>
> // We should not try to destroy an applet during
> @@ -547,6 +553,10 @@
>
> // Else set to given status
> status.put(identifier, newStatus);
> +
> + synchronized(status) {
> + status.notifyAll();
> + }
>
Similar comment as above.
> return prev;
> }
> @@ -599,7 +609,9 @@
> }
>
> /**
> - * Function to block until applet initialization is complete
> + * 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.
> */
> @@ -608,16 +620,14 @@
> int waitTime = 0;
>
> // Wait till initialization finishes
> - while (panel.getApplet() == null &&
> - panel.isAlive() &&
> - waitTime < APPLET_TIMEOUT) {
> - try {
> - if (waitTime % 500 == 0)
> - PluginDebug.debug("Waiting for applet panel ", panel, " to initialize...");
> + long maxTimeToSleep = APPLET_TIMEOUT;
>
> - Thread.sleep(waitTime += 50);
> - } catch (InterruptedException ie) {
> - // just wait
> + synchronized(panel) {
> + while (panel.getApplet() == null &&
> + panel.isAlive() &&
> + maxTimeToSleep > 0) {
> + PluginDebug.debug("Waiting for applet panel ", panel, " to initialize...");
> + maxTimeToSleep -= waitTillTimeout(panel, maxTimeToSleep);
> }
> }
>
> @@ -628,14 +638,11 @@
> if (message.startsWith("width")) {
>
> // Wait for panel to come alive
> - int maxWait = APPLET_TIMEOUT; // wait for panel to come alive
> - int wait = 0;
> - while (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) && wait < maxWait) {
> - try {
> - Thread.sleep(50);
> - wait += 50;
> - } 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?
> }
> }
>
> @@ -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.
> }
>
> // Wait for the panel to initialize
> @@ -2049,4 +2060,36 @@
> // Draw the painted image
> g.drawImage(bufFrameImg, 0, 0, this);
> }
> +
> + /**
> + * Waits on a given object 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
> + * @param timeout The maximum time to wait
> + * @return Approximate time spent sleeping (not guaranteed to be perfect)
> + */
> + public static long waitTillTimeout(Object obj, long timeout) {
> +
> + // Can't wait on null. Return 0 indicating no wait happened.
> + if (obj == null)
> + return 0;
> +
> + // Record when we started sleeping
> + long sleepStart = System.currentTimeMillis();
> +
> + try {
> + obj.wait(timeout);
> + } catch (InterruptedException ie) {} // Discarded, time to return
> +
> + // Record when we woke up
> + long sleepEnd = System.currentTimeMillis();
> +
> + // Return the difference
> + return sleepEnd - sleepStart;
> + }
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. :-)
> }
--
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
More information about the distro-pkg-dev
mailing list