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

[Omair Majid]

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?

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.

Thanks,
Omair