ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1 #787

athanatos · 2019-01-25T20:49:47Z

Add test for cxid rollover to 1
Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1.

eolivelli · 2019-01-25T20:51:54Z

Interesting.
Are you interested in a back port to 3.4, 3.5?

athanatos · 2019-01-25T20:53:29Z

Yeah, I've got the 3.4 one ready, 3.5 presumably also easy.

eolivelli

awesome work @athanatos

I left a couple of comments, please take a look

eolivelli · 2019-01-25T21:17:58Z

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java

     * the server. Thus, getXid() must be public.
     */
    synchronized public int getXid() {
+        // xid values of -4, -2, and -1 are special, see SendThread.readResponse


What about introducing constants for these special values ? (and replace in readResponse)

Where do protocol level constants like that usually get put?

Also, that seems a bit intrusive for a patch I want to backport since it would also touch the server side. I suppose I could add it as an additional commit and only backport the actual fix?

I am thinking about having constants only locally to this file, this way it will be easier to understand the code and the patch wouldn't be so intrusive.
In the (hopefully near) future we will separate client size code from server side code so introducing new shared constants would be overkilling.

I don't fell strong about having such constants, it is only a thought/suggestion.

cc @anmolnar @lvfangmin @hanm

I'm inclined to keep the patch minimal since grepping for those identifiers wouldn't tell you where those packets get sent. Also, the numerical values are useful to see here. I don't feel strongly about it though.

no problem from my side, let's keep the patch simple

eolivelli · 2019-01-25T22:02:20Z

zookeeper-server/src/test/java/org/apache/zookeeper/TestableZooKeeper.java

+        }
+    }
+
+    protected ClientCnxn createConnection(String chrootPath,


what about overriding this method only in your new testcase?

I figured that the xid manipulation methods might be generally useful. Also, createClient test helper method explicitly creates a TestableZooKeeper, so I'd have to create another version of that, I think. Doesn't seem worthwhile.

eolivelli

LGTM
thank you

+1 (non binding)

athanatos · 2019-01-29T17:55:50Z

Is this waiting on something from me?

eolivelli · 2019-01-29T18:22:45Z

We need two committers to review and then merge.

Tagging @anmolnar

athanatos · 2019-02-08T22:44:28Z

Anything I can do to move this along?

eolivelli · 2019-02-09T06:54:06Z

@anmolnar can we include this in 3.5.5 and 3.4.14?

anmolnar · 2019-02-11T11:36:43Z

@eolivelli 3.5.5 is fine, but 3.4.14 is already cut, I wouldn't touch it anymore.

We can make an exception if it's critical, but we need at least one more committer to review.

nkalmar

Nice and simple fix, good test addition.

+1, thanks!

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java

athanatos · 2019-02-25T19:10:50Z

Ok, I pushed a version that wraps from MAX to 1 avoiding negative values entirely. @phunt

anmolnar

+1 lgtm

phunt · 2019-02-27T16:52:59Z

So here's my current concern, which I have not had time yet to look at and I think should be addressed before this is committed - what happens on the server side? Is it allowing the cxid to "go back in time", if so is that ok, if not then what happens? (e.g. does it disco the client session?)

athanatos · 2019-02-27T17:51:51Z

@phunt I was unable to find anything server side that actually compares cxids other than to check equality. I believe this patch to be safe so long as two requests with the same cxid on the same session are not live at the same time -- which a 31 bit cxid is probably sufficient to ensure. Note, the current implementation already permits wrapping from MAX to MIN. This behavior occurs regularly in our environment and the only misbehavior it seems to cause is that the requests submitted with a cxid value of -4, -2, or -1 hang or cause a reconnect. The patch also includes a test to exercise the behavior specifically.

athanatos · 2019-02-27T17:54:13Z

Pushed to fix the commit message -- forgot to update it before.

phunt

+1 looks good to me with a minor nit that should be addressed. I will then commit this.

We want this to go into master, seems like we should also commit to at least 3.5? Anyone with strong opinions re 3.4? (I would probably lean towards commiting there as well)

phunt · 2019-03-03T17:27:58Z

zookeeper-server/src/test/java/org/apache/zookeeper/test/ClientTest.java

+                        }
+                    }, null);
+                Assert.assertTrue("setData should complete within 5s",
+                    latch.await(5, TimeUnit.SECONDS));


this is generally an anti-pattern in the tests and leads to flakey tests. e.g. we can see on some test environments that are oversubscribed. I'd suggest something very large, e.g. 30 seconds or just using the session timeout.

- Add test for cxid rollover to 1 - Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1.

athanatos · 2019-03-04T19:49:51Z

@phunt I pushed a version with that changed. The JenkinsMaven failure doesn't appear related to this patch.

phunt · 2019-03-06T02:18:22Z

+1, lgtm. thanks @athanatos

I also reviewed the server side code and it looks like client xid "going back in time" (my earlier concern) is fine.

I did notice this however - notice negative client xids circumvent throttling! So we're fixing this bug as well by resetting the xid before it rolls over.

    public void incrOutstandingAndCheckThrottle(RequestHeader h) {
        if (h.getXid() <= 0) {
            return;
        }
        if (zkServer.shouldThrottle(outstandingCount.incrementAndGet())) {
            disableRecv(false);
        }
    }

- Add test for cxid rollover to 1 - Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1. Author: Samuel Just <sjust@salesforce.com> Reviewers: phunt@apache.org Closes #787 from athanatos/forupstream/ZOOKEEPER-3253 Change-Id: Ib3d111170bb086d6982f2cf0ee5cf8afd5157588 (cherry picked from commit e10c93a) Signed-off-by: Patrick Hunt <phunt@apache.org>

- Add test for cxid rollover to 1 - Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1. Author: Samuel Just <sjust@salesforce.com> Reviewers: phunt@apache.org Closes apache#787 from athanatos/forupstream/ZOOKEEPER-3253 Change-Id: Ib3d111170bb086d6982f2cf0ee5cf8afd5157588 (cherry picked from commit e10c93a) Includes backport of createConnection testability refactor from 9f82798. Signed-off-by: Samuel Just <sjust@salesforce.com>

- Add test for cxid rollover to 1 - Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1. Author: Samuel Just <sjustsalesforce.com> Reviewers: phuntapache.org Closes #787 from athanatos/forupstream/ZOOKEEPER-3253 Change-Id: Ib3d111170bb086d6982f2cf0ee5cf8afd5157588 (cherry picked from commit e10c93a) Includes backport of createConnection testability refactor from 9f82798. Signed-off-by: Samuel Just <sjustsalesforce.com> Author: Samuel Just <sjust@salesforce.com> Reviewers: phunt@apache.org Closes #844 from athanatos/forupstream/ZOOKEEPER-3235-3.4

- Add test for cxid rollover to 1 - Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1. Author: Samuel Just <sjust@salesforce.com> Reviewers: phunt@apache.org Closes apache#787 from athanatos/forupstream/ZOOKEEPER-3253 Change-Id: Ib3d111170bb086d6982f2cf0ee5cf8afd5157588

eolivelli requested changes Jan 25, 2019

View reviewed changes

eolivelli reviewed Jan 25, 2019

View reviewed changes

eolivelli approved these changes Jan 25, 2019

View reviewed changes

athanatos mentioned this pull request Feb 6, 2019

[bk-gc] Fix GC thread gets blocked apache/bookkeeper#1937

Merged

nkalmar approved these changes Feb 20, 2019

View reviewed changes

phunt reviewed Feb 25, 2019

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java Outdated Show resolved Hide resolved

athanatos force-pushed the forupstream/ZOOKEEPER-3253 branch from 49e93a5 to 3cd4e8a Compare February 25, 2019 19:09

anmolnar approved these changes Feb 26, 2019

View reviewed changes

athanatos force-pushed the forupstream/ZOOKEEPER-3253 branch from 3cd4e8a to 381d167 Compare February 27, 2019 17:52

phunt approved these changes Mar 3, 2019

View reviewed changes

ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1

904cc30

- Add test for cxid rollover to 1 - Modify ClientCnxn.SendThread.getXid() to increment from MAX to 1.

athanatos force-pushed the forupstream/ZOOKEEPER-3253 branch from 381d167 to 904cc30 Compare March 4, 2019 19:21

asfgit closed this in e10c93a Mar 6, 2019

athanatos mentioned this pull request Mar 8, 2019

ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1 #844

Closed

rhdxmr mentioned this pull request Mar 10, 2020

Fix client transaction id overflow micro-fan/aiozk#50

Merged

ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1 #787

ZOOKEEPER-3253: client should not send requests with cxid=-4, -2, or -1 #787

Uh oh!

Conversation

athanatos commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli commented Jan 25, 2019

Uh oh!

athanatos commented Jan 25, 2019

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

athanatos commented Jan 29, 2019

Uh oh!

eolivelli commented Jan 29, 2019

Uh oh!

athanatos commented Feb 8, 2019

Uh oh!

eolivelli commented Feb 9, 2019

Uh oh!

anmolnar commented Feb 11, 2019

Uh oh!

nkalmar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

athanatos commented Feb 25, 2019

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

phunt commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athanatos commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athanatos commented Feb 27, 2019

Uh oh!

phunt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athanatos commented Mar 4, 2019

Uh oh!

phunt commented Mar 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

athanatos commented Jan 25, 2019 •

edited

Loading

phunt commented Feb 27, 2019 •

edited

Loading

athanatos commented Feb 27, 2019 •

edited

Loading