Tony Wu 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 @@
*
*/
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.
