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 xu.regis@gmail.com designates
 74.125.92.149 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:content-type:content-transfer-encoding;
        b=UUdaFjrKbxvGnfPONu1u8DFMaagunTcb0ey5iH6EjubYVQQ+SifwEHO/d+nc+AES7E
         n2BMeI2q84KZUX+PwM9vr0eystf5leYPbT64o9ZupuIc+Awoz6QzPF6XVVQ4KkZjhgOW
         UbyQOoahnRdNPiCOPpxrvXkzOEb5fum7xnI+M=
Message-ID: <496ED2B1.5080800@gmail.com>
Date: Thu, 15 Jan 2009 14:07:45 +0800
From: Regis <xu.regis@gmail.com>
User-Agent: Thunderbird 2.0.0.19 (Windows/20081209)
MIME-Version: 1.0
To: dev@harmony.apache.org
Subject: Re: [classlib][nio]Is selector.close() blocking or not?
References: <dcd1e0ad0901122017n390fcd56sf5dbf00a51b9f89@mail.gmail.com>
	 <496C5092.3070701@gmail.com>
	 <211709bc0901140034h6b8bac67ncfdf3f7e852a1d3d@mail.gmail.com>
	 <496DAAE9.5090805@gmail.com>
	 <211709bc0901141847l77826f9biaf3879921751cc@mail.gmail.com>
	 <3b3f27c60901142105gdd9d42bu4ca42f7caae63b35@mail.gmail.com>
 <211709bc0901142157h2007191cn3dfe24e293381122@mail.gmail.com>
 <496ED282.4020009@gmail.com>
In-Reply-To: <496ED282.4020009@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Regis wrote:
> Tony Wu wrote:
>> On Thu, Jan 15, 2009 at 1:05 PM, Nathan Beyer <ndbeyer@apache.org> wrote:
>>> On Wed, Jan 14, 2009 at 4:47 PM, Tony Wu <wuyuehao@gmail.com> wrote:
>>>> On Wed, Jan 14, 2009 at 5:05 PM, Regis <xu.regis@gmail.com> wrote:
>>>>> Tony Wu wrote:
>>>>>> On Tue, Jan 13, 2009 at 4:28 PM, Regis <xu.regis@gmail.com> wrote:
>>>>>>> Hui Ding wrote:
>>>>>>>> Hi Experts,
>>>>>>>> I am writing an application with NIO and Selector and found a 
>>>>>>>> strange
>>>>>>>> issue that if I call "Selector.close()" while a thread is being
>>>>>>>> blocked on that Selector(By calling "Selector.select()"), the 
>>>>>>>> call to
>>>>>>>> "close()" will block indefinitely until "Selector.select()" 
>>>>>>>> returns.
>>>>>>>> It's said in the Javadoc of selector.close() that: "If a thread is
>>>>>>>> currently blocked in one of this selector's selection methods 
>>>>>>>> then it
>>>>>>>> is interrupted as if by invoking the selector's wakeup method". 
>>>>>>>> So I
>>>>>>>> guess under no circumstances should Select.close() block, right?
>>>>>>>> However I came across another topic about the concurrency of 
>>>>>>>> Selector:
>>>>>>>> "The selection operations synchronize on the selector itself, on 
>>>>>>>> the
>>>>>>>> key set, and on the selected-key set, in that order.
>>>>>>>> The close method synchronizes on the selector and all three key 
>>>>>>>> sets
>>>>>>>> in the same order as in a selection operation." If it is true that
>>>>>>>> Selector.select() and Selector.close() both synchronize on the
>>>>>>>> selector itself, I think Selector.close() will block if other 
>>>>>>>> threads
>>>>>>>> are blocked on Selector.select().
>>>>>>>>
>>>>>>>> Now I am confused about which one is the correct behavior of
>>>>>>>> Selector.close(), blocking or not?
>>>>>>>>
>>>>>>>> BTW. I gave it a try on a Desktop PC with SUN JDK and Harmony 
>>>>>>>> JDK, seems
>>>>>>>> with SUN JDK, Selector.close() will never block while with Harmony
>>>>>>>> JDK, if another thread is being blocked on Selector.select(),
>>>>>>>> Selector.close will block until select() returns. Is this a bug 
>>>>>>>> with
>>>>>>>> Harmony JDK or not?
>>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Could you help to try the following patch?
>>>>>>>
>>>>>>> Index:
>>>>>>>
>>>>>>> modules/nio/src/main/java/common/java/nio/channels/spi/AbstractSelector.java 
>>>>>>>
>>>>>>> ===================================================================== 
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> modules/nio/src/main/java/common/java/nio/channels/spi/AbstractSelector.java 
>>>>>>>
>>>>>>> +++
>>>>>>>
>>>>>>> modules/nio/src/main/java/common/java/nio/channels/spi/AbstractSelector.java 
>>>>>>>
>>>>>>> @@ -58,7 +58,7 @@ public abstract class AbstractSelector extends 
>>>>>>> Selector
>>>>>>> {
>>>>>>>     * @see java.nio.channels.Selector#close()
>>>>>>>     */
>>>>>>>    @Override
>>>>>>> -    public synchronized final void close() throws IOException {
>>>>>>> +    public final void close() throws IOException {
>>>>>>>        if (isOpen) {
>>>>>>>            isOpen = false;
>>>>>>>            implCloseSelector();
>>>>>> Hi Regis,
>>>>>>
>>>>>> I've not run with this patch but I think there is a pontential 
>>>>>> risk of
>>>>>> running implCloseSelector() more than once. with patch above, several
>>>>>> threads might enter the "if" block in the same time if they all want
>>>>>> to close the Selector. and then the wakeup() and deregister() will be
>>>>>> called many times.
>>>>> Yes, but for wakeup(), according to the spec:
>>>>> "Invoking this method more than once between two successive selection
>>>>> operations has the same effect as invoking it just once."
>>>>> so I think it's OK to invoke it many times.
>>>>>
>>>>> And for deregister(), it should be invoke only once. Because it is 
>>>>> protected
>>>>> by these locks, once the first thread enter the block and execute
>>>>> successfully, the "keys" should be empty, so deregister() won't be 
>>>>> called
>>>>> any more. doCancel() is the similar case.
>>>> it is protected by locks but could be invoked one by one, right? what
>>>> about following patch
>>>>
>>>> ### Eclipse Workspace Patch 1.0
>>>> #P nio
>>>> Index: META-INF/MANIFEST.MF
>>>> ===================================================================
>>>> --- META-INF/MANIFEST.MF        (revision 720103)
>>>> +++ META-INF/MANIFEST.MF        (working copy)
>>>> @@ -19,6 +19,7 @@
>>>>  java.nio.charset,
>>>>  java.security,
>>>>  java.util,
>>>> + java.util.concurrent.atomic,
>>>>  org.apache.harmony.kernel.vm,
>>>>  org.apache.harmony.luni.net,
>>>>  org.apache.harmony.luni.platform,
>>>> Index: src/main/java/common/java/nio/channels/spi/AbstractSelector.java
>>>> ===================================================================
>>>> --- 
>>>> src/main/java/common/java/nio/channels/spi/AbstractSelector.java    
>>>> (revision
>>>> 720103)
>>>> +++ 
>>>> src/main/java/common/java/nio/channels/spi/AbstractSelector.java    
>>>> (working
>>>> copy)
>>>> @@ -22,6 +22,7 @@
>>>>  import java.nio.channels.Selector;
>>>>  import java.util.HashSet;
>>>>  import java.util.Set;
>>>> +import java.util.concurrent.atomic.AtomicBoolean;
>>>>
>>>>  /**
>>>>  * Abstract class for selectors.
>>>> @@ -33,7 +34,7 @@
>>>>  *
>>>>  */
>>> How is this different?
>>
>> atomic class extends the feature of volatile, and it supports
>> lock-free algorithm. For example, the getAndSet method cost only one
>> instruction if the processor supports.
>>>>  public abstract class AbstractSelector extends Selector {
>>>> -    private volatile boolean isOpen = true;
>>>> +    private AtomicBoolean isOpen = new AtomicBoolean(true);
>>>>
>>>>     private SelectorProvider provider = null;
>>>>
>>>> @@ -59,8 +60,7 @@
>>>>      */
>>> This probably shouldn't have been synchronized, since the field was 
>>> volatile.
>>
>> in this case, if we do not use atomic, we have to synchronize it. take
>> following code as an example
>>
>> if(isOpen){ #1
>>  // #2
>>  isOpen = false; // #3
>> }
>>
>> when thread 1 has just finished the #1, but have not finished #3,
>> thread 2 has the chance to enter this block since the read and write
>> operation are not completed in one instruction.
>>
>>>>     @Override
>>>>     public synchronized final void close() throws IOException {
>>>> -        if (isOpen) {
>>>> -            isOpen = false;
>>>> +        if (isOpen.getAndSet(false)) {
>>>>             implCloseSelector();
>>>>         }
>>>>     }
>>>> @@ -78,7 +78,7 @@
>>>>      */
>>> This method could have just been synchronized and the field's volatile
>>> modifier could be removed.
>>
>> agreed. this synchronized should have been removed.
>>
>>>>     @Override
>>>>     public final boolean isOpen() {
>>>> -        return isOpen;
>>>> +        return isOpen.get();
>>>>     }
>>>>
>>>>     /**
>>>>
>>>>
>>>>
>>>>>>> Index:
>>>>>>>
>>>>>>> modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java 
>>>>>>>
>>>>>>> ===================================================================== 
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java 
>>>>>>>
>>>>>>> +++
>>>>>>>
>>>>>>> modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java 
>>>>>>>
>>>>>>> @@ -138,6 +138,7 @@ final class SelectorImpl extends 
>>>>>>> AbstractSelector {
>>>>>>>     */
>>>>>>>    @Override
>>>>>>>    protected void implCloseSelector() throws IOException {
>>>>>>> +        wakeup();
>>>>>>>        synchronized (this) {
>>>>>>>            synchronized (keysSet) {
>>>>>>>                synchronized (selectedKeys) {
>>>>>>> @@ -147,7 +148,6 @@ final class SelectorImpl extends 
>>>>>>> AbstractSelector {
>>>>>>>                            deregister((AbstractSelectionKey) sk);
>>>>>>>                        }
>>>>>>>                    }
>>>>>>> -                    wakeup();
>>>>>>>                }
>>>>>>>            }
>>>>>>>        }
>>>>>>>
>>>>>>>
>>>>>>> after the patch, Harmony could pass the following test as RI:
>>>>>>>
>>>>>>>       Thread thread = new Thread(new Runnable() {
>>>>>>>           public void run() {
>>>>>>>               int i = 0;
>>>>>>>               try {
>>>>>>>                   if (selector.select() != 0) {
>>>>>>>                       throw new RuntimeException("invalid return 
>>>>>>> value: "
>>>>>>> +
>>>>>>> i);
>>>>>>>                   }
>>>>>>>               } catch (IOException e) {
>>>>>>>                   // TODO Auto-generated catch block
>>>>>>>                   e.printStackTrace();
>>>>>>>               }
>>>>>>>           }
>>>>>>>       });
>>>>>>>       thread.start();
>>>>>>>       Thread.sleep(1000);
>>>>>>>
>>>>>>>       selector.close();
>>>>>>>       System.out.println("selector is closed");
>>>>>>>
>>>>>>> -- 
>>>>>>> Best Regards,
>>>>>>> Regis.
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> -- 
>>>>> Best Regards,
>>>>> Regis.
>>>>>
>>>>
>>>>
>>>> -- 
>>>> Tony Wu
>>>> China Software Development Lab, IBM
>>>>
>>
>>
>>
> I have filed a JIRA [1] for this issue, and attach a patch which merge 
> patches from Tony and me. Would anyone want to look at it?
> 

Sorry, forgot the JIRA link:

https://issues.apache.org/jira/browse/HARMONY-6073

-- 
Best Regards,
Regis.