harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tony Wu" <wuyue...@gmail.com>
Subject Re: [classlib][nio]Is selector.close() blocking or not?
Date Thu, 15 Jan 2009 05:57:22 GMT
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
>>
>



-- 
Tony Wu
China Software Development Lab, IBM

Mime
View raw message