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

[Jiri Vanek]

On 04/14/2011 05:04 PM, Deepak Bhole wrote:
> * 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
>
Don't forgot then that nanoTime can be negative.