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 03:42:11 GMT
Tony Wu 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 @@
>   *
>   */
>  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 @@
>       */
>      @Override
>      public synchronized final void close() throws IOException {
> -        if (isOpen) {
> -            isOpen = false;
> +        if (isOpen.getAndSet(false)) {
>              implCloseSelector();
>          }
>      }
> @@ -78,7 +78,7 @@
>       */
>      @Override
>      public final boolean isOpen() {
> -        return isOpen;
> +        return isOpen.get();
>      }
> 
>      /**
> 
> 
> 
I like this way :) so synchronize close() method is not necessary
And I think HARMONY-6014 could be fixed by the similar way

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


-- 
Best Regards,
Regis.

Mime
View raw message