Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: pass (athena.apache.org: domain of t.p.ellison@gmail.com
 designates 209.85.134.190 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:user-agent:mime-version:to:subject:references:in-reply-to:x-enigmail-version:content-type:content-transfer-encoding;
        b=trWXU9DqJ9IYn8vUDC68n0qIDgB8V+xz+c2B4ZqMnTfiMK/HwPi3EENA/JDLU8NK+hZazqE6VZWgVT7EjK9xI/kOneAYILqXHr3/H7/HHbtHlwU6eEO8s7J3WSOoEO5/PWd5/7Bdzg4YgZxCeQ92YCApxh4eyBP8//MWLG6gpUA=
Message-ID: <475B0D4C.5000905@gmail.com>
Date: Sat, 08 Dec 2007 21:31:56 +0000
From: Tim Ellison <t.p.ellison@gmail.com>
User-Agent: Thunderbird 2.0.0.9 (Windows/20071031)
MIME-Version: 1.0
To: dev@harmony.apache.org
Subject: Re: [classlib][nio] jetty corrupts big static files
References: <2c9597b90711290606l48fd13bfj240ca335ce067b5a@mail.gmail.com>
	 <2c9597b90711290640t207fc92du5bc350ed98f23dba@mail.gmail.com>
	 <4bebff790711290643m1a626933qf0c1bc86f1d0b207@mail.gmail.com>
	 <2c9597b90711290650s658a53c8y2fe035021a4fbf40@mail.gmail.com>
	 <4bebff790711290750n51322cdbg99e76a206d019842@mail.gmail.com>
	 <4bebff790711300452l6824f2fawd09adbf4d5f0d17f@mail.gmail.com>
	 <475562F3.6040102@gmail.com>
	 <4bebff790712040630k476729bbk637b8cf3c5b7963b@mail.gmail.com>
	 <475823D7.9010700@gmail.com>
	 <200712070912.lB79CrMB019518@d12av04.megacenter.de.ibm.com>
 <4bebff790712070317g1eea13a4q4f4a3ec4fe429eed@mail.gmail.com>
 <47595128.406@gmail.com>
In-Reply-To: <47595128.406@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Given that Aleksey and I came up with essentially the same patch to help
alleviate this instability -- is there any support for putting it into
the code now (ahead of M4 testing)?  I believe it is a relatively
localized and minor change even at this stage.

Regards,
Tim


Tim Ellison wrote:
> Aleksey Shipilev wrote:
>> Hi, Mark!
>>
>> On Dec 7, 2007 12:12 PM, Mark Hindess <mark.hindess@googlemail.com> wrote:
>>>> It looks like the code in NIO selector could do with a good review,
>>>> and at minimum a few comments if not a partial redesign to tidy up the
>>>> data structures and which locks are required for which, etc.
>>> I agree.  Though it is tempting to revert the HARMONY-4869 optimizations
>>> (see JIRA comment for the two svn diff commands to do this) since that
>>> does seem marginally more stable to me.
>> I wonder if fixing the current legacy selector will be easier - that's
>> because HARMONY-4869 already has some tidying in data structures. I
>> suspect there are synchronization problems while modifying the key
>> already registered on the selector. Can you try wrap the modKey() [1]
>> with synchronization as follows:
>>
>>     void modKey(SelectionKey sk) {
>> +         synchronized (this) {
>> +          synchronized (keysSet) {
>>      	    	// TODO: update indexes rather than recreate the key
>>      	    	delKey(sk);
>>      	     	addKey(sk);
>> +             }
>> +         }
>>     }
>>
>> ...and see the difference?
> 
> 
> I came to the same conclusion and have been running with the following
> patch for a while... I'll admit that I followed other places in grabbing
> the selectedkeys lock too, and it may not be necessary:
> 
> Index: main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java
> ===================================================================
> --- main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java
> (revision 602067)
> +++ main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java
> (working copy)
> @@ -359,8 +359,14 @@
>       */
>      void modKey(SelectionKey sk) {
>          // TODO: update indexes rather than recreate the key
> -        delKey(sk);
> -        addKey(sk);
> +        synchronized (this) {
> +            synchronized (keysSet) {
> +                synchronized (selectedKeys) {
> +                    delKey(sk);
> +                    addKey(sk);
> +                }
> +            }
> +        }
>      }
> 
>      /**
> @@ -578,6 +584,9 @@
>          return unaddableSelectedKeys;
>      }
> 
> +    /*
> +     * Assumes calling thread holds locks on 'this', 'keysSet', and
> 'selectedKeys'.
> +     */
>      private void doCancel() {
>          Set<SelectionKey> cancelledKeys = cancelledKeys();
>          synchronized (cancelledKeys) {
>