Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@zookeeper.apache.org
From: rakeshadr <git@git.apache.org>
To: dev@zookeeper.apache.org
Reply-To: dev@zookeeper.apache.org
References: <git-pr-254-zookeeper@git.apache.org>
In-Reply-To: <git-pr-254-zookeeper@git.apache.org>
Subject: [GitHub] zookeeper pull request #254: ZOOKEEPER-2775: ZK Client not able to connect w...
Content-Type: text/plain
Message-Id: <20170518100005.A73AADFE61@git1-us-west.apache.org>
Date: Thu, 18 May 2017 10:00:05 +0000 (UTC)
archived-at: Thu, 18 May 2017 10:00:07 -0000

Github user rakeshadr commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/254#discussion_r117207576
  
    --- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
    @@ -1054,6 +1054,8 @@ private void sendPing() {
             private boolean saslLoginFailed = false;
     
             private void startConnect() throws IOException {
    +            // initializing it for new connection
    +            saslLoginFailed = false;
    --- End diff --
    
    Thanks @arshadmohammad  for the details.
    
    yes, only `SendThread` is updating the flag. But, during sasl login retries period, the flag status will be checked by `tunnelAuthInProgress()` packet processing thread, so multiple threads are accessing the flag. The code looks little tricky and `zooKeeperSaslClient `null value represents auth in progress. I'm almost OK with the change and trying another attempt to avoid any compatibility issues to the users as this would go to stable branches:-). 
    
    Earlier the behavior was, once the flag updated to flase, `tunnelAuthInProgress` function would return false always. Now, with the proposed fix, sometimes it would return false and sometimes it would return true, right? Will this results in any consistency issues later?
    
    Assume  a case, where successful login takes several retries.
    (1) Immediately after the login failure the flag will be false. During this time `tunnelAuthInProgress() ` function returns false to the callers.
    (2) Assume, `startConnect()` retries started. During this time, `tunnelAuthInProgress() ` function returns true to the callers.
    
    My previous suggestion was to avoid this situation and consistently `tunnelAuthInProgress()` function return false until the next successful login. Does this makes sense to you?
    
    @hanm, welcome your thoughts. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---