harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nathan Beyer <ndbe...@apache.org>
Subject Re: [classlib][nio]Is selector.close() blocking or not?
Date Thu, 15 Jan 2009 05:05:40 GMT
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?

>  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.

>     @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.

>     @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
>

Mime
View raw message