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 02:47:51 GMT
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();
     }

     /**



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