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:06:58 GMT
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?

-- 
Best Regards,
Regis.

Mime
View raw message