You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by abhishek-chouhan <gi...@git.apache.org> on 2018/06/11 17:36:34 UTC
[GitHub] zookeeper pull request #541: ZOOKEEPER-3059 EventThread leak in case of Sasl...
GitHub user abhishek-chouhan opened a pull request:
https://github.com/apache/zookeeper/pull/541
ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFailed
Since authFailed is similar to session expired and is considered a fatal event, we should clean up after ourselves once we get a AuthFailed, other wise this results in an unavoidable and un-cleanable thread leak of EventThread since the close operation is also a no-op (we return after checking for isAlive).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/abhishek-chouhan/zookeeper master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/541.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #541
----
commit c1d9d7afe1de246ad32d43032df7bb428fcceae6
Author: Abhishek Singh Chouhan <ab...@...>
Date: 2018-06-11T17:31:42Z
ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFailed
----
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/541
@abhishek-chouhan No problem, your test looking good. You've added `eventOfDeath` to 2 places in the code: line 824 and line 1168:
When I remove 824, your test will fail which is good. But if I remove 1168, test will still pass.
Under what conditions do we need the change at line 1168 and could you write unit test for that?
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by abhishek-chouhan <gi...@git.apache.org>.
Github user abhishek-chouhan commented on the issue:
https://github.com/apache/zookeeper/pull/541
@anmolnar is there a way i could re trigger jenkins?
---
[GitHub] zookeeper pull request #541: ZOOKEEPER-3059 EventThread leak in case of Sasl...
Posted by abhishek-chouhan <gi...@git.apache.org>.
Github user abhishek-chouhan closed the pull request at:
https://github.com/apache/zookeeper/pull/541
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by abhishek-chouhan <gi...@git.apache.org>.
Github user abhishek-chouhan commented on the issue:
https://github.com/apache/zookeeper/pull/541
@maoling thanks, i also looked at the test failure and it looked unrelated and from the history of the test looked like a flapper. Have re triggered a test run anyways.
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by abhishek-chouhan <gi...@git.apache.org>.
Github user abhishek-chouhan commented on the issue:
https://github.com/apache/zookeeper/pull/541
@anmolnar The second one happens when zooKeeperSaslClient.initialize(ClientCnxn.this) throws an sasl exception, which from the code looks like can happen in case of incomplete initialization. In zookeeperSaslClient's constructor we call createSaslClient which in case of anything apart from LoginException will swallow the exception and would initialize the saslClient object as null(which would cause initialize to throw sasl exception in the null check). It however looks hard to simulate an exception in the initialization part, since there are static methods involved and stubbing zookeepersaslclient itself doesn't look to be an option.
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/541
Got it. Let's trigger a green build and I'll commit.
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/541
Committed to master and 3.5 branches. Thanks @abhishek-chouhan
---
[GitHub] zookeeper pull request #541: ZOOKEEPER-3059 EventThread leak in case of Sasl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/541
---
[GitHub] zookeeper pull request #541: ZOOKEEPER-3059 EventThread leak in case of Sasl...
Posted by abhishek-chouhan <gi...@git.apache.org>.
GitHub user abhishek-chouhan reopened a pull request:
https://github.com/apache/zookeeper/pull/541
ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFailed
Since authFailed is similar to session expired and is considered a fatal event, we should clean up after ourselves once we get a AuthFailed, other wise this results in an unavoidable and un-cleanable thread leak of EventThread since the close operation is also a no-op (we return after checking for isAlive).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/abhishek-chouhan/zookeeper master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/541.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #541
----
commit c1d9d7afe1de246ad32d43032df7bb428fcceae6
Author: Abhishek Singh Chouhan <ab...@...>
Date: 2018-06-11T17:31:42Z
ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFailed
commit c54a83a486e4b6372e7004d44d2dbd0ba7ab61bb
Author: Abhishek Singh Chouhan <ab...@...>
Date: 2018-06-22T03:12:18Z
ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFailed. Adding testcase for the scenario
----
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by abhishek-chouhan <gi...@git.apache.org>.
Github user abhishek-chouhan commented on the issue:
https://github.com/apache/zookeeper/pull/541
Finally got a green build :)
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by abhishek-chouhan <gi...@git.apache.org>.
Github user abhishek-chouhan commented on the issue:
https://github.com/apache/zookeeper/pull/541
@anmolnar Sorry for the delay. Have written a test case for this. Do have a look.
---
[GitHub] zookeeper issue #541: ZOOKEEPER-3059 EventThread leak in case of Sasl AuthFa...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/541
@abhishek-chouhan close this PR and reopen it will retrigger jenkins.It seems the failures isn't related with this PR
---