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

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2011-04-14 10:56]:
> On 04/14/2011 01:56 AM, 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).
> >
> >Cheers,
> >Deepak
> >
> >
> >wait-fixes.patch
> >
> >
> >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);
> 
> Can you not use waitTillTimeout here?
> 

Nope, it is in a different class. That is why I added a comment above it
stating why it was done so. To use that, I would have to make
PluginAppletViewer.waitTillTimeout() public and I would rather not
expose that for use elsewhere in its current state. I am working on a
refactor to address it correctly.

> Also, perhaps we should be using System.nanoTime() instead of
> System.currentTimeMillis().
> 
> Using System.currenTimeMillis() relies on the assumption that the
> user's clock does not change. If a daylight savings change happens
> to kick in, or the computer's clock is updated (say, based on NTP),
> then the results of currentTimeMillis() can be quite disastrous
> (especially if the clock moved backwards).
> 
> System.nanoTime() uses clock_gettime(CLOCK_MONOTONIC) (where
> supported) so it should be safe from any changes in the clock.
> 

Ah, neat. I didn't know that. I will make the change.

Thanks,
Deepak