Mailing-List: contact commits-help@zookeeper.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@zookeeper.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: hanm@apache.org
To: commits@zookeeper.apache.org
Message-Id: <0bd6ac7465354778b0eee3856d9e11b3@git.apache.org>
Subject: zookeeper git commit: ZOOKEEPER-2743: Netty connection leaks JMX
 connection bean.
Date: Mon, 17 Apr 2017 23:44:32 +0000 (UTC)
archived-at: Mon, 17 Apr 2017 23:44:34 -0000

Repository: zookeeper
Updated Branches:
  refs/heads/branch-3.5 212201131 -> 434fecb11


ZOOKEEPER-2743: Netty connection leaks JMX connection bean.

See https://issues.apache.org/jira/browse/ZOOKEEPER-2743 for details on the symptom and diagnostic.

There are many ways of fixing this. For example we can enforce certain ordering of the close operations to eliminate the race condition however that would incur non trivial performance penalty as a result of synchronization. I think the solution in this patch is simple and effective as it observes that the bean unregister call is idempotent and the call itself is trivial so we just always unregister connection upon close the connection, to ensure the bean is unregistered regardless of the races.

I've also stress tested this solution on internal Jenkins which indicates no failures of https://issues.apache.org/jira/browse/ZOOKEEPER-2707 for the past two days.

A side note is NIO connection in theory would suffer the same problem however I am unable to reproduce the same racing with existing unit test. So I just leave NIO as it is, for now.

Author: Michael Han <hanm@apache.org>

Reviewers: Rakesh Radhakrishnan <rakeshr@apache.org>

Closes #211 from hanm/ZOOKEEPER-2743


Project: http://git-wip-us.apache.org/repos/asf/zookeeper/repo
Commit: http://git-wip-us.apache.org/repos/asf/zookeeper/commit/434fecb1
Tree: http://git-wip-us.apache.org/repos/asf/zookeeper/tree/434fecb1
Diff: http://git-wip-us.apache.org/repos/asf/zookeeper/diff/434fecb1

Branch: refs/heads/branch-3.5
Commit: 434fecb1192c7c5968fa396ab217447716a44d7f
Parents: 2122011
Author: Michael Han <hanm@apache.org>
Authored: Mon Apr 10 10:19:34 2017 -0700
Committer: Michael Han <hanm@apache.org>
Committed: Mon Apr 17 16:44:24 2017 -0700

----------------------------------------------------------------------
 .../main/org/apache/zookeeper/server/NettyServerCnxn.java     | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/zookeeper/blob/434fecb1/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
----------------------------------------------------------------------
diff --git a/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java b/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
index cf5bd8a..142e916 100644
--- a/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
+++ b/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
@@ -87,6 +87,12 @@ public class NettyServerCnxn extends ServerCnxn {
             LOG.debug("close called for sessionid:0x"
                     + Long.toHexString(sessionId));
         }
+
+        // ZOOKEEPER-2743:
+        // Always unregister connection upon close to prevent
+        // connection bean leak under certain race conditions.
+        factory.unregisterConnection(this);
+
         synchronized(factory.cnxns){
             // if this is not in cnxns then it's already closed
             if (!factory.cnxns.remove(this)) {
@@ -111,7 +117,6 @@ public class NettyServerCnxn extends ServerCnxn {
         if (channel.isOpen()) {
             channel.close();
         }
-        factory.unregisterConnection(this);
     }
 
     @Override