From commits-return-7671-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Fri Mar 8 04:09:25 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 0FA3F180654 for ; Fri, 8 Mar 2019 05:09:24 +0100 (CET) Received: (qmail 16695 invoked by uid 500); 8 Mar 2019 04:09:24 -0000 Mailing-List: contact commits-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list commits@zookeeper.apache.org Received: (qmail 16684 invoked by uid 99); 8 Mar 2019 04:09:23 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 08 Mar 2019 04:09:23 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 5294287974; Fri, 8 Mar 2019 04:09:23 +0000 (UTC) Date: Fri, 08 Mar 2019 04:09:23 +0000 To: "commits@zookeeper.apache.org" Subject: [zookeeper] branch branch-3.4 updated: ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <155201816318.21357.6423523133764025922@gitbox.apache.org> From: phunt@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: zookeeper X-Git-Refname: refs/heads/branch-3.4 X-Git-Reftype: branch X-Git-Oldrev: a2a62a9fa12c90b43fd4faa5f48508e6aa4b9557 X-Git-Newrev: 6604f36d93d7d73b8d6e8e20164b33ece691f408 X-Git-Rev: 6604f36d93d7d73b8d6e8e20164b33ece691f408 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated This is an automated email from the ASF dual-hosted git repository. phunt pushed a commit to branch branch-3.4 in repository https://gitbox.apache.org/repos/asf/zookeeper.git The following commit(s) were added to refs/heads/branch-3.4 by this push: new 6604f36 ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1 6604f36 is described below commit 6604f36d93d7d73b8d6e8e20164b33ece691f408 Author: Samuel Just AuthorDate: Thu Mar 7 20:08:13 2019 -0800 ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1 - Add test for cxid rollover to 1 - Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1. Author: Samuel Just Reviewers: phuntapache.org Closes #787 from athanatos/forupstream/ZOOKEEPER-3253 Change-Id: Ib3d111170bb086d6982f2cf0ee5cf8afd5157588 (cherry picked from commit e10c93a590cc1b73eebad48d18cfcbceb3ec0d4d) Includes backport of createConnection testability refactor from 9f82798415351a20136ceb1640b1781723e51cc1. Signed-off-by: Samuel Just Author: Samuel Just Reviewers: phunt@apache.org Closes #844 from athanatos/forupstream/ZOOKEEPER-3235-3.4 --- .../main/java/org/apache/zookeeper/ClientCnxn.java | 9 +++++- .../main/java/org/apache/zookeeper/ZooKeeper.java | 10 ++++++- .../org/apache/zookeeper/TestableZooKeeper.java | 34 +++++++++++++++++++++ .../java/org/apache/zookeeper/test/ClientTest.java | 35 ++++++++++++++++++++++ 4 files changed, 86 insertions(+), 2 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java index dfd036e..8d0faf2 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java @@ -1383,7 +1383,8 @@ public class ClientCnxn { } } - private int xid = 1; + // @VisibleForTesting + protected int xid = 1; // @VisibleForTesting volatile States state = States.NOT_CONNECTED; @@ -1393,6 +1394,12 @@ public class ClientCnxn { * the server. Thus, getXid() must be public. */ synchronized public int getXid() { + // Avoid negative cxid values. In particular, cxid values of -4, -2, and -1 are special and + // must not be used for requests -- see SendThread.readResponse. + // Skip from MAX to 1. + if (xid == Integer.MAX_VALUE) { + xid = 1; + } return xid++; } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java index 03c8b0d..529efcc 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java @@ -448,12 +448,20 @@ public class ZooKeeper { connectString); HostProvider hostProvider = new StaticHostProvider( connectStringParser.getServerAddresses()); - cnxn = new ClientCnxn(connectStringParser.getChrootPath(), + cnxn = createConnection(connectStringParser.getChrootPath(), hostProvider, sessionTimeout, this, watchManager, getClientCnxnSocket(), canBeReadOnly); cnxn.start(); } + // @VisibleForTesting + protected ClientCnxn createConnection(String chrootPath, + HostProvider hostProvider, int sessionTimeout, ZooKeeper zooKeeper, + ClientWatchManager watcher, ClientCnxnSocket clientCnxnSocket, + boolean canBeReadOnly) throws IOException { + return new ClientCnxn(chrootPath, hostProvider, sessionTimeout, this, + watchManager, clientCnxnSocket, canBeReadOnly); + } /** * To create a ZooKeeper client object, the application needs to pass a * connection string containing a comma separated list of host:port pairs, diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java b/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java index dd70056..7407c52 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java @@ -25,6 +25,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import org.apache.jute.Record; +import org.apache.zookeeper.client.HostProvider; import org.apache.zookeeper.proto.ReplyHeader; import org.apache.zookeeper.proto.RequestHeader; @@ -34,6 +35,39 @@ public class TestableZooKeeper extends ZooKeeper { Watcher watcher) throws IOException { super(host, sessionTimeout, watcher); } + + class TestableClientCnxn extends ClientCnxn { + TestableClientCnxn(String chrootPath, HostProvider hostProvider, int sessionTimeout, ZooKeeper zooKeeper, + ClientWatchManager watcher, ClientCnxnSocket clientCnxnSocket, boolean canBeReadOnly) + throws IOException { + super(chrootPath, hostProvider, sessionTimeout, zooKeeper, watcher, + clientCnxnSocket, 0, new byte[16], canBeReadOnly); + } + + void setXid(int newXid) { + xid = newXid; + } + + int checkXid() { + return xid; + } + } + + protected ClientCnxn createConnection(String chrootPath, + HostProvider hostProvider, int sessionTimeout, ZooKeeper zooKeeper, + ClientWatchManager watcher, ClientCnxnSocket clientCnxnSocket, + boolean canBeReadOnly) throws IOException { + return new TestableClientCnxn(chrootPath, hostProvider, sessionTimeout, this, + watcher, clientCnxnSocket, canBeReadOnly); + } + + public void setXid(int xid) { + ((TestableClientCnxn)cnxn).setXid(xid); + } + + public int checkXid() { + return ((TestableClientCnxn)cnxn).checkXid(); + } @Override public List getChildWatches() { diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientTest.java index 9dc4861..c973597 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientTest.java @@ -26,6 +26,8 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.zookeeper.AsyncCallback; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.KeeperException.Code; @@ -802,4 +804,37 @@ public class ClientTest extends ClientBase { Assert.assertTrue("failed to disconnect", clientDisconnected.await(5000, TimeUnit.MILLISECONDS)); } + + @Test + public void testCXidRollover() throws Exception { + TestableZooKeeper zk = null; + try { + zk = createClient(); + zk.setXid(Integer.MAX_VALUE - 10); + + zk.create("/testnode", "".getBytes(), Ids.OPEN_ACL_UNSAFE, + CreateMode.PERSISTENT); + for (int i = 0; i < 20; ++i) { + final CountDownLatch latch = new CountDownLatch(1); + final AtomicInteger rc = new AtomicInteger(0); + zk.setData("/testnode", "".getBytes(), -1, + new AsyncCallback.StatCallback() { + @Override + public void processResult(int retcode, String path, Object ctx, Stat stat) { + rc.set(retcode); + latch.countDown(); + } + }, null); + Assert.assertTrue("setData should complete within 5s", + latch.await(zk.getSessionTimeout(), TimeUnit.MILLISECONDS)); + Assert.assertEquals("setData should have succeeded", Code.OK.intValue(), rc.get()); + } + zk.delete("/testnode", -1); + Assert.assertTrue("xid should be positive", zk.checkXid() > 0); + } finally { + if (zk != null) { + zk.close(); + } + } + } }