This is an automated email from the ASF dual-hosted git repository. hanm pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/master by this push: new f9610cc ZOOKEEPER-2894: Memory and completions leak on zookeeper_close f9610cc is described below commit f9610cc80173342bbe9766889a1aab1bfd840d1e Author: Alexander A. Strelets <strelet...@gmail.com> AuthorDate: Mon Jul 8 17:22:15 2019 -0700 ZOOKEEPER-2894: Memory and completions leak on zookeeper_close If I call `zookeeper_close()` when the call-stack does not pass through any of zookeeper mechanics (for example, some of completion handler called back by zookeeper) the following happens: 1. If there are requests waiting for responses in `zh.sent_requests` queue, they all are removed from this queue and each of them is "completed" with personal fake response having status `ZCLOSING`. Such fake responses are put into `zh.completions_to_process` queue. It's Ok 2. But then, `zh.completions_to_process` queue is left unhandled. **Neither completion callbacks are called, nor dynamic memory allocated for fake responses is freed** 3. Different structures within `zh` are dismissed and finally `zh` is freed Consequently we have a tiny **memory leak** and, which is much more important, **completions leak**. I.e. completions are not called at all. This is not the case if I call `zookeeper_close()` when the call-stack does pass through some of zookeeper mechanics. Here, zookeeper client handles completions properly. Proposed changes remove this defects. For more details see: https://issues.apache.org/jira/browse/ZOOKEEPER-2894 Author: Alexander A. Strelets <strelet...@gmail.com> Reviewers: Enrico Olivelli <eolive...@gmail.com>, Michael Han <h...@apache.org> Closes #1000 from xoiss/ZOOKEEPER-2894 --- .../zookeeper-client-c/src/zookeeper.c | 3 + .../zookeeper-client-c/tests/TestOperations.cc | 115 +++++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/zookeeper-client/zookeeper-client-c/src/zookeeper.c b/zookeeper-client/zookeeper-client-c/src/zookeeper.c index 544c436..70762c2 100644 --- a/zookeeper-client/zookeeper-client-c/src/zookeeper.c +++ b/zookeeper-client/zookeeper-client-c/src/zookeeper.c @@ -566,6 +566,9 @@ static void destroy(zhandle_t *zh) } /* call any outstanding completions with a special error code */ cleanup_bufs(zh,1,ZCLOSING); + if (process_async(zh->outstanding_sync)) { + process_completions(zh); + } if (zh->hostname != 0) { free(zh->hostname); zh->hostname = NULL; diff --git a/zookeeper-client/zookeeper-client-c/tests/TestOperations.cc b/zookeeper-client/zookeeper-client-c/tests/TestOperations.cc index b8a4b3f..8890e1f 100644 --- a/zookeeper-client/zookeeper-client-c/tests/TestOperations.cc +++ b/zookeeper-client/zookeeper-client-c/tests/TestOperations.cc @@ -32,6 +32,8 @@ class Zookeeper_operations : public CPPUNIT_NS::TestFixture CPPUNIT_TEST(testUnsolicitedPing); CPPUNIT_TEST(testTimeoutCausedByWatches1); CPPUNIT_TEST(testTimeoutCausedByWatches2); + CPPUNIT_TEST(testCloseWhileInProgressFromMain); + CPPUNIT_TEST(testCloseWhileInProgressFromCompletion); #else CPPUNIT_TEST(testAsyncWatcher1); CPPUNIT_TEST(testAsyncGetOperation); @@ -444,6 +446,119 @@ public: CPPUNIT_ASSERT_EQUAL((int32_t)TIMEOUT/3*1000,toMilliseconds(now-beginningOfTimes)); } + // ZOOKEEPER-2894: Memory and completions leak on zookeeper_close + // while there is a request waiting for being processed + // call zookeeper_close() from the main event loop + // assert the completion callback is called + void testCloseWhileInProgressFromMain() + { + Mock_gettimeofday timeMock; + ZookeeperServer zkServer; + CloseFinally guard(&zh); + + zh=zookeeper_init("localhost:2121",watcher,10000,TEST_CLIENT_ID,0,0); + CPPUNIT_ASSERT(zh!=0); + forceConnected(zh); + zhandle_t* savezh=zh; + + // issue a request + zkServer.addOperationResponse(new ZooGetResponse("1",1)); + AsyncGetOperationCompletion res1; + int rc=zoo_aget(zh,"/x/y/1",0,asyncCompletion,&res1); + CPPUNIT_ASSERT_EQUAL((int)ZOK,rc); + + // but do not allow Zookeeper C Client to process the request + // and call zookeeper_close() from the main event loop immediately + Mock_free_noop freeMock; + rc=zookeeper_close(zh); zh=0; + freeMock.disable(); + CPPUNIT_ASSERT_EQUAL((int)ZOK,rc); + + // verify that memory for completions was freed (would be freed if no mock installed) + CPPUNIT_ASSERT_EQUAL(1,freeMock.getFreeCount(savezh)); + CPPUNIT_ASSERT(savezh->completions_to_process.head==0); + CPPUNIT_ASSERT(savezh->completions_to_process.last==0); + + // verify that completion was called, and it was called with ZCLOSING status + CPPUNIT_ASSERT(res1.called_); + CPPUNIT_ASSERT_EQUAL((int)ZCLOSING,res1.rc_); + } + + // ZOOKEEPER-2894: Memory and completions leak on zookeeper_close + // send some request #1 + // then, while there is a request #2 waiting for being processed + // call zookeeper_close() from the completion callback of request #1 + // assert the completion callback #2 is called + void testCloseWhileInProgressFromCompletion() + { + Mock_gettimeofday timeMock; + ZookeeperServer zkServer; + CloseFinally guard(&zh); + + zh=zookeeper_init("localhost:2121",watcher,10000,TEST_CLIENT_ID,0,0); + CPPUNIT_ASSERT(zh!=0); + forceConnected(zh); + zhandle_t* savezh=zh; + + // will handle completion on request #1 and issue request #2 from it + class AsyncGetOperationCompletion1: public AsyncCompletion{ + public: + AsyncGetOperationCompletion1(zhandle_t **zh, ZookeeperServer *zkServer, + AsyncGetOperationCompletion *res2) + :zh_(zh),zkServer_(zkServer),res2_(res2){} + + virtual void dataCompl(int rc1, const char *value, int len, const Stat *stat){ + CPPUNIT_ASSERT_EQUAL((int)ZOK,rc1); + + // from the completion #1 handler, issue request #2 + zkServer_->addOperationResponse(new ZooGetResponse("2",1)); + int rc2=zoo_aget(*zh_,"/x/y/2",0,asyncCompletion,res2_); + CPPUNIT_ASSERT_EQUAL((int)ZOK,rc2); + + // but do not allow Zookeeper C Client to process the request #2 + // and call zookeeper_close() from the completion callback of request #1 + rc2=zookeeper_close(*zh_); *zh_=0; + CPPUNIT_ASSERT_EQUAL((int)ZOK,rc2); + + // do not disable freeMock here, let completion #2 handler + // return through ZooKeeper C Client internals to the main loop + // and fulfill the work + } + + zhandle_t **zh_; + ZookeeperServer *zkServer_; + AsyncGetOperationCompletion *res2_; + }; + + // issue request #1 + AsyncGetOperationCompletion res2; + AsyncGetOperationCompletion1 res1(&zh,&zkServer,&res2); + zkServer.addOperationResponse(new ZooGetResponse("1",1)); + int rc=zoo_aget(zh,"/x/y/1",0,asyncCompletion,&res1); + CPPUNIT_ASSERT_EQUAL((int)ZOK,rc); + + // process the send queue + int fd; int interest; timeval tv; + rc=zookeeper_interest(zh,&fd,&interest,&tv); + CPPUNIT_ASSERT_EQUAL((int)ZOK,rc); + CPPUNIT_ASSERT(zh!=0); + Mock_free_noop freeMock; + while(zh!=0 && (rc=zookeeper_process(zh,interest))==ZOK) { + millisleep(100); + } + freeMock.disable(); + CPPUNIT_ASSERT(zh==0); + + // verify that memory for completions was freed (would be freed if no mock installed) + CPPUNIT_ASSERT_EQUAL(1,freeMock.getFreeCount(savezh)); + CPPUNIT_ASSERT(savezh->completions_to_process.head==0); + CPPUNIT_ASSERT(savezh->completions_to_process.last==0); + + // verify that completion #2 was called, and it was called with ZCLOSING status + CPPUNIT_ASSERT(res2.called_); + CPPUNIT_ASSERT_EQUAL((int)ZCLOSING,res2.rc_); + } + #else class TestGetDataJob: public TestJob{ public: