[
https://issues.apache.org/jira/browse/DIRMINA-1076?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16360060#comment-16360060
]
Jonathan Valliere edited comment on DIRMINA-1076 at 2/12/18 12:38 AM:
----------------------------------------------------------------------
*Notes:*
I've thought about it and have come up with the following patch which resolves
my question about the previous patch. I was attempting to find a solution
which made the smallest number of changes to resolve the problem. As it turns
out, moving the {{isDisposing()}} check above {{removeSessions()}} resolves the
double-removal issue due to blind removal scheduling based on the Selectors
{{keys()}}.
I believe the method in the following patch to be safe due to the nature of the
disposing state; since disposal is immediate and we don't care about letting
Sessions soft-close there is no need to wakeup the Selector.
*What was happening:*
* {{process(Session)}} would cause self remove via {{scheduleRemoval}}
* {{removeSessions}} removes all sessions scheduled via {{scheduleRemoval}}
* {{isDisposing()}} block would add ALL sessions from {{selector.keys()}} to
{{scheduleRemoval}} causing the previously self removed session to be
scheduled, again, using {{scheduleRemoval}}. ({{selector.keys()}} does not
update until the next time {{select()}} is called.) The next time
{{removeSessions}} is called, the session counter goes negative. The
self-destruct routine checks for ({{session count == 0)}} which never is
satisfied because the session count is negative number.
This patch works by moving the {{isDisposing()}} block before
{{removeSessions}} utilizing the {{contains(Session)}} check of
{{removeSessions}} to prevent a single {{Session}} from being added to the list
twice.
_Patch to prevent double-removal of a single session which causes the counter
to go negative preventing the processor from disposing._
{code:java}
diff --git
a/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
b/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
index 79885fa..2715b98 100644
---
a/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
+++
b/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
@@ -659,9 +659,20 @@
long currentTime = System.currentTimeMillis();
flush(currentTime);
+ // Disconnect all sessions immediately if disposal has been
+ // requested so that we exit this loop eventually.
+ if (isDisposing()) {
+ for (Iterator<S> i = allSessions(); i.hasNext();) {
+ IoSession session = i.next();
+ scheduleRemove((S) session);
+ }
+ }
+
// And manage removed sessions
nSessions -= removeSessions();
-
+
+ assert nSessions > -1 : "Internal Session Count is
Negative";
+
// Last, not least, send Idle events to the idle sessions
notifyIdleSessions(currentTime);
@@ -685,26 +696,6 @@
}
assert processorRef.get() == this;
- }
-
- // Disconnect all sessions immediately if disposal has been
- // requested so that we exit this loop eventually.
- if (isDisposing()) {
- boolean hasKeys = false;
-
- for (Iterator<S> i = allSessions(); i.hasNext();) {
- IoSession session = i.next();
-
- scheduleRemove((S) session);
-
- if (session.isActive()) {
- hasKeys = true;
- }
- }
-
- if (hasKeys) {
- wakeup();
- }
}
} catch (ClosedSelectorException cse) {
// If the selector has been closed, we can exit the loop
{code}
I had to change Christoph's {{AbstractIoServiceTest}} patch because it calls
{{connector.dispose(true)}} which the documentation and comments above say is
something that should not be done within an {{IoFutureListener}}. The reason I
had to change from {{true}} to {{false}} was because {{dispose(true)}} blocks
on itself and preventing the {{IoProcessor}} thread from ever stopping. In the
described test, this causes the application to create threads until the OS
complains about it preventing more threads from being created.
_Patch to make the test run in a loop_
{code:java}
diff --git
a/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
b/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
index 2d70f8e..325c4e4 100644
---
a/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
+++
b/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
@@ -30,6 +30,7 @@
import org.apache.mina.filter.logging.LoggingFilter;
import org.apache.mina.transport.socket.nio.NioSocketAcceptor;
import org.apache.mina.transport.socket.nio.NioSocketConnector;
+import org.apache.mina.util.AvailablePortFinder;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -48,16 +49,15 @@
*/
public class AbstractIoServiceTest {
- private static final int PORT = 9123;
-
@Test
public void testDispose() throws IOException, InterruptedException {
+ while ( true) {
List<String> threadsBefore = getThreadNames();
final IoAcceptor acceptor = new NioSocketAcceptor();
- acceptor.getFilterChain().addLast("logger", new LoggingFilter());
+ // acceptor.getFilterChain().addLast("logger", new LoggingFilter());
acceptor.getFilterChain().addLast("codec",
new ProtocolCodecFilter(new
TextLineCodecFactory(Charset.forName("UTF-8"))));
@@ -65,7 +65,8 @@
acceptor.getSessionConfig().setReadBufferSize(2048);
acceptor.getSessionConfig().setIdleTime(IdleStatus.BOTH_IDLE, 10);
- acceptor.bind(new InetSocketAddress(PORT));
+ int nextAvailable = AvailablePortFinder.getNextAvailable();
+ acceptor.bind(new InetSocketAddress(nextAvailable));
System.out.println("Server running ...");
final NioSocketConnector connector = new NioSocketConnector();
@@ -74,12 +75,12 @@
connector.setConnectTimeoutMillis(30 * 1000L);
connector.setHandler(new ClientHandler());
- connector.getFilterChain().addLast("logger", new LoggingFilter());
+ // connector.getFilterChain().addLast("logger", new LoggingFilter());
connector.getFilterChain().addLast("codec",
new ProtocolCodecFilter(new
TextLineCodecFactory(Charset.forName("UTF-8"))));
// Start communication.
- ConnectFuture cf = connector.connect(new
InetSocketAddress("localhost", 9123));
+ ConnectFuture cf = connector.connect(new
InetSocketAddress("localhost", nextAvailable));
cf.awaitUninterruptibly();
IoSession session = cf.getSession();
@@ -103,7 +104,9 @@
public void operationComplete(IoFuture future) {
System.out.println("managed session count=" +
connector.getManagedSessionCount());
System.out.println("Disposing connector ...");
- connector.dispose(true);
+ // the doc states that the following should not be called with
parameter TRUE from an IoFutureListener?!
+ // on the other hand, using FALSE does not work either
+ connector.dispose(false);
System.out.println("Disposing connector ... *finished*");
}
@@ -114,11 +117,11 @@
List<String> threadsAfter = getThreadNames();
- System.out.println("threadsBefore = " + threadsBefore);
- System.out.println("threadsAfter = " + threadsAfter);
+ // System.out.println("threadsBefore = " + threadsBefore);
+ // System.out.println("threadsAfter = " + threadsAfter);
// Assert.assertEquals(threadsBefore, threadsAfter);
-
+ }
}
public static class ClientHandler extends IoHandlerAdapter {
{code}
was (Author: johnnyv):
*Notes:*
I've thought about it and have come up with the following patch which resolves
my question about the previous patch. I was attempting to find a solution
which made the smallest number of changes to resolve the problem. As it turns
out, moving the {{isDisposing()}} check above {{removeSessions()}} resolves the
double-removal issue due to blind removal scheduling based on the Selectors
{{keys()}}.
I believe the method in the following patch to be safe due to the nature of the
disposing state; since disposal is immediate and we don't care about letting
Sessions soft-close there is no need to wakeup the Selector.
*What was happening:*
* {{process(Session)}} would cause self remove via {{scheduleRemoval}}
* {{removeSessions}} removes all sessions scheduled via {{scheduleRemoval}}
* {{isDisposing()}} block would add ALL sessions from {{selector.keys()}} to
{{scheduleRemoval}} causing the previously self removed session to be
scheduled, again, using {{scheduleRemoval}}. The next time {{removeSessions}}
is called, the session counter goes negative. The self-destruct routine checks
for ({{session count == 0)}} which never is satisfied because the session count
is negative number.
This patch works by moving the {{isDisposing()}} block before
{{removeSessions}} utilizing the {{contains(Session)}} check of
{{removeSessions}} to prevent a single {{Session}} from being added to the list
twice.
_Patch to prevent double-removal of a single session which causes the counter
to go negative preventing the processor from disposing._
{code:java}
diff --git
a/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
b/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
index 79885fa..2715b98 100644
---
a/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
+++
b/mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
@@ -659,9 +659,20 @@
long currentTime = System.currentTimeMillis();
flush(currentTime);
+ // Disconnect all sessions immediately if disposal has been
+ // requested so that we exit this loop eventually.
+ if (isDisposing()) {
+ for (Iterator<S> i = allSessions(); i.hasNext();) {
+ IoSession session = i.next();
+ scheduleRemove((S) session);
+ }
+ }
+
// And manage removed sessions
nSessions -= removeSessions();
-
+
+ assert nSessions > -1 : "Internal Session Count is
Negative";
+
// Last, not least, send Idle events to the idle sessions
notifyIdleSessions(currentTime);
@@ -685,26 +696,6 @@
}
assert processorRef.get() == this;
- }
-
- // Disconnect all sessions immediately if disposal has been
- // requested so that we exit this loop eventually.
- if (isDisposing()) {
- boolean hasKeys = false;
-
- for (Iterator<S> i = allSessions(); i.hasNext();) {
- IoSession session = i.next();
-
- scheduleRemove((S) session);
-
- if (session.isActive()) {
- hasKeys = true;
- }
- }
-
- if (hasKeys) {
- wakeup();
- }
}
} catch (ClosedSelectorException cse) {
// If the selector has been closed, we can exit the loop
{code}
I had to change Christoph's {{AbstractIoServiceTest}} patch because it calls
{{connector.dispose(true)}} which the documentation and comments above say is
something that should not be done within an {{IoFutureListener}}. The reason I
had to change from {{true}} to {{false}} was because {{dispose(true)}} blocks
on itself and preventing the {{IoProcessor}} thread from ever stopping. In the
described test, this causes the application to create threads until the OS
complains about it preventing more threads from being created.
_Patch to make the test run in a loop_
{code:java}
diff --git
a/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
b/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
index 2d70f8e..325c4e4 100644
---
a/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
+++
b/mina-core/src/test/java/org/apache/mina/core/service/AbstractIoServiceTest.java
@@ -30,6 +30,7 @@
import org.apache.mina.filter.logging.LoggingFilter;
import org.apache.mina.transport.socket.nio.NioSocketAcceptor;
import org.apache.mina.transport.socket.nio.NioSocketConnector;
+import org.apache.mina.util.AvailablePortFinder;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -48,16 +49,15 @@
*/
public class AbstractIoServiceTest {
- private static final int PORT = 9123;
-
@Test
public void testDispose() throws IOException, InterruptedException {
+ while ( true) {
List<String> threadsBefore = getThreadNames();
final IoAcceptor acceptor = new NioSocketAcceptor();
- acceptor.getFilterChain().addLast("logger", new LoggingFilter());
+ // acceptor.getFilterChain().addLast("logger", new LoggingFilter());
acceptor.getFilterChain().addLast("codec",
new ProtocolCodecFilter(new
TextLineCodecFactory(Charset.forName("UTF-8"))));
@@ -65,7 +65,8 @@
acceptor.getSessionConfig().setReadBufferSize(2048);
acceptor.getSessionConfig().setIdleTime(IdleStatus.BOTH_IDLE, 10);
- acceptor.bind(new InetSocketAddress(PORT));
+ int nextAvailable = AvailablePortFinder.getNextAvailable();
+ acceptor.bind(new InetSocketAddress(nextAvailable));
System.out.println("Server running ...");
final NioSocketConnector connector = new NioSocketConnector();
@@ -74,12 +75,12 @@
connector.setConnectTimeoutMillis(30 * 1000L);
connector.setHandler(new ClientHandler());
- connector.getFilterChain().addLast("logger", new LoggingFilter());
+ // connector.getFilterChain().addLast("logger", new LoggingFilter());
connector.getFilterChain().addLast("codec",
new ProtocolCodecFilter(new
TextLineCodecFactory(Charset.forName("UTF-8"))));
// Start communication.
- ConnectFuture cf = connector.connect(new
InetSocketAddress("localhost", 9123));
+ ConnectFuture cf = connector.connect(new
InetSocketAddress("localhost", nextAvailable));
cf.awaitUninterruptibly();
IoSession session = cf.getSession();
@@ -103,7 +104,9 @@
public void operationComplete(IoFuture future) {
System.out.println("managed session count=" +
connector.getManagedSessionCount());
System.out.println("Disposing connector ...");
- connector.dispose(true);
+ // the doc states that the following should not be called with
parameter TRUE from an IoFutureListener?!
+ // on the other hand, using FALSE does not work either
+ connector.dispose(false);
System.out.println("Disposing connector ... *finished*");
}
@@ -114,11 +117,11 @@
List<String> threadsAfter = getThreadNames();
- System.out.println("threadsBefore = " + threadsBefore);
- System.out.println("threadsAfter = " + threadsAfter);
+ // System.out.println("threadsBefore = " + threadsBefore);
+ // System.out.println("threadsAfter = " + threadsAfter);
// Assert.assertEquals(threadsBefore, threadsAfter);
-
+ }
}
public static class ClientHandler extends IoHandlerAdapter {
{code}
> Leaking NioProcessors/NioSocketConnectors hanging in call to dispose
> --------------------------------------------------------------------
>
> Key: DIRMINA-1076
> URL: https://issues.apache.org/jira/browse/DIRMINA-1076
> Project: MINA
> Issue Type: Bug
> Affects Versions: 2.0.16
> Reporter: Christoph John
> Priority: Major
> Attachments: mina-dispose-hang.txt, mina-test-log.txt,
> mina-test-patch.txt
>
>
> Follow-up to mailing list discussion.
> I was now able to reproduce the problem with a MINA test. Or let's say I did
> the brute-force approach by re-running one test in an endless loop.
> I have attached a patch of AbstractIoServiceTest (against
> [https://github.com/apache/mina/tree/2.0]) and a stack trace. After a few
> loops the test is stuck. You can see a lot of threads hanging in dispose()
> and the test is stuck when it tries to dispose the acceptor.
>
> What is a little strange is that the javadoc says that
> connector.dispose(TRUE) should not be called from an IoFutureListener, but in
> the test it is done anyway. However, changing the parameter to FALSE does not
> help either.
>
> Is there anything that can be done to prevent this hang?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
