harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Regis <xu.re...@gmail.com>
Subject Re: [classlib][nio]Is selector.close() blocking or not?
Date Thu, 15 Jan 2009 06:07:45 GMT
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.

Mime
View raw message