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 Fri, 16 Jan 2009 05:40:18 GMT
patch has been integrated at r734911. Thanks to all of you.

On Thu, Jan 15, 2009 at 2:07 PM, Regis <xu.regis@gmail.com> wrote:
> Regis wrote:
>>
>> 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?
>>
>
> Sorry, forgot the JIRA link:
>
> https://issues.apache.org/jira/browse/HARMONY-6073
>
> --
> Best Regards,
> Regis.
>



-- 
Tony Wu
China Software Development Lab, IBM

Mime
View raw message