patch has been integrated at r734911. Thanks to all of you. On Thu, Jan 15, 2009 at 2:07 PM, Regis <[email protected]> wrote: > Regis wrote: >> >> Tony Wu wrote: >>> >>> On Thu, Jan 15, 2009 at 1:05 PM, Nathan Beyer <[email protected]> wrote: >>>> >>>> On Wed, Jan 14, 2009 at 4:47 PM, Tony Wu <[email protected]> wrote: >>>>> >>>>> On Wed, Jan 14, 2009 at 5:05 PM, Regis <[email protected]> wrote: >>>>>> >>>>>> Tony Wu wrote: >>>>>>> >>>>>>> On Tue, Jan 13, 2009 at 4:28 PM, Regis <[email protected]> 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
