[rfc][icedtea-web] function applet -> js ->applet call deadlock

[Jiri Vanek]

On 05/17/2013 05:10 PM, Adam Domurad wrote:
> On 05/17/2013 07:04 AM, Jiri Vanek wrote:
>>
>> Here you go:)
>>
>>
>> J.
>>
>> On 05/15/2013 05:55 PM, Adam Domurad wrote:
>>> On 05/15/2013 07:56 AM, Jiri Vanek wrote:
>>>> When applet calls JavaScript function, which calls back to appelt,
>>>> then *mostly* deadlock occurs.
>>>> I made several attempts to fix this more generally, but I was
>>>> unsuccessful.
>>>>
>>>> So this "hack" is preventing deadlock, maybe also the timeout can be
>>>> a bit shorter...
>>>>
>>>> Although this is for both head and 1.4, for head some more
>>>> investigations are needed later.
>>>>
>>>> J.
>>>>
>>>> ps,reproducer in progress.
>>>
>>> Hi, thanks for looking into it. Probably good idea to remove
>>> dead-lock potential.
>>>
>>>> diff -r 9f2d8381f5f1
>>>> plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>>>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Fri
>>>> May 03 16:17:08 2013 +0200
>>>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Mon
>>>> May 06 16:06:59 2013 +0200
>>>> @@ -1305,8 +1305,16 @@
>>>> PluginDebug.debug("wait ToString request 1");
>>>> synchronized (request) {
>>>> PluginDebug.debug("wait ToString request 2");
>>>> - while (request.isDone() == false)
>>>> - request.wait();
>>>> + int counter = 0;
>>>> + while (request.isDone() == false){
>>>> + // Do not wait indefinitely to avoid the potential of deadlock
>>>> + // but this will destroy the intentional recursion ?
>>>> + counter++;
>>>> + if (counter>10){
>>>> + throw new InterruptedException("Possible deadlock, releasing");
>>>> + }
>>>> + request.wait(1000);
>>>> + }
>>>> PluginDebug.debug("wait ToString request 3");
>>>> }
>>>> } catch (InterruptedException e) {
>>>>
>>>
>>> This is more complex than it needs to be.
>>> More simple is:
>>>
>>> if (!request.isDone()) {
>>> request.wait(REQUEST_TIMEOUT);
>>> }
>>> if (!request.isDone()) {
>>> // Do not wait indefinitely to avoid the potential of deadlock
>>> throw new RuntimeException("Possible deadlock, releasing");
>>> }
>>>
>>> Your message gets tossed aside and a RuntimeException is thrown if
>>> you throw InterruptedException,
>>> more direct is better.
>>>
>>> Also please put this in its own method, eg waitForRequestCompletion
>>> (probably good to encapsulate
>>> the 'catch InterruptedException' here). There are many methods like
>>> this that have a wait loop.
>>> Though I would like to see the reproducer before commenting more.
>>>
>>> Thanks,
>>> -Adam
>>>
>>
> 
> RE: the newest fix (well, fix for dead-lock)
> 
>> diff -r 1b1e547ccb4a
>> plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Wed May
>> 15 12:14:26 2013 +0200
>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Fri May
>> 17 13:02:36 2013 +0200
>> @@ -181,6 +181,18 @@
>>
>>
>> private SplashPanel splashPanel;
>> +
>> + private static long REQUEST_TIMEOUT=2000;//2s
> 
> I am a little fearful of legitimate requests that take >2 seconds. There
> are some monstrous Javascript functions out there; this timeout is a
> hard limit on arbitrary Javascript code. I'd sleep easier if it were at
> least a minute (I know this would break the reproducer, but you could
> set the time-out longer). We're just asking for confusing regressions
> otherwise (at least if we *do* cause a regression with Javascript code
> that takes a minute, it will be fairly obvious).

Hmhm. No issue with reproducer of course. At first I had 10s and it sounded to me like an ages...

Do you really wont such an long timeout?

Although I see your concern.... my bargain is going to 20s.. Ouch I would love 10.. but it is to low
under your offer :)

The vote against the "monstrous Javascript functions out there" :)) is that js->applet->js should
not be such an evil construction,  Well - js->applet or applet-> js can be... but... Well Its the
same loop isnt it?
Then your minute will probably win.

(reproducer need update now, but after your reply)


> 
>> +
>> + private static void waitForRequestCompletion(PluginCallRequest
>> request) throws RuntimeException, InterruptedException {
> 
> You don't need to declare you're throwing RuntimeException, and I don't
> think it's in good style.

:DDD Sorry, good catch!
> Also you can encapsulate the 'catch (InterruptedException)' rethrow
> logic here.

As runtime exception? Probably good idea.
> 
>> + if (!request.isDone()) {
>> + request.wait(REQUEST_TIMEOUT);
>> + }
>> + if (!request.isDone()) {
>> + // Do not wait indefinitely to avoid the potential of deadlock
>> + throw new RuntimeException("Possible deadlock, releasing");
>> + }
>> + }
>>
>> /**
>> * Null constructor to allow instantiation via newInstance()
>> @@ -1305,8 +1317,7 @@
>> PluginDebug.debug("wait ToString request 1");
>> synchronized (request) {
>> PluginDebug.debug("wait ToString request 2");
>> - while (request.isDone() == false)
>> - request.wait();
>> + waitForRequestCompletion(request);
>> PluginDebug.debug("wait ToString request 3");
>> }
>> } catch (InterruptedException e) {
> 
> Surely this method should be used anywhere that 'while (request.isDone()
> == false)' was used. Unless you see a reason it shouldn't.

As a separate changese unless you insist.
Refactoring should always happen as separate changeset. Ok?


-------------- next part --------------
A non-text attachment was scrubbed...
Name: dadlockFix-fix2.patch
Type: text/x-patch
Size: 2038 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130517/0d0916bc/dadlockFix-fix2.patch