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?
--
Best Regards,
Regis.
