Skip to content

Commit 5bf8cf1

Browse files
xoisshanm
xoiss
authored and
hanm
committed
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 <streletsaa@gmail.com> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Michael Han <hanm@apache.org> Closes apache#1000 from xoiss/ZOOKEEPER-2894
1 parent 4a5d596 commit 5bf8cf1

File tree

2 files changed

+118
-0
lines changed

2 files changed

+118
-0
lines changed

‎zookeeper-client/zookeeper-client-c/src/zookeeper.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,9 @@ static void destroy(zhandle_t *zh)
418418
}
419419
/* call any outstanding completions with a special error code */
420420
cleanup_bufs(zh,1,ZCLOSING);
421+
if (process_async(zh->outstanding_sync)) {
422+
process_completions(zh);
423+
}
421424
if (zh->hostname != 0) {
422425
free(zh->hostname);
423426
zh->hostname = NULL;

‎zookeeper-client/zookeeper-client-c/tests/TestOperations.cc

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ class Zookeeper_operations : public CPPUNIT_NS::TestFixture
3232
CPPUNIT_TEST(testUnsolicitedPing);
3333
CPPUNIT_TEST(testTimeoutCausedByWatches1);
3434
CPPUNIT_TEST(testTimeoutCausedByWatches2);
35+
CPPUNIT_TEST(testCloseWhileInProgressFromMain);
36+
CPPUNIT_TEST(testCloseWhileInProgressFromCompletion);
3537
#else
3638
CPPUNIT_TEST(testAsyncWatcher1);
3739
CPPUNIT_TEST(testAsyncGetOperation);
@@ -444,6 +446,119 @@ class Zookeeper_operations : public CPPUNIT_NS::TestFixture
444446
CPPUNIT_ASSERT_EQUAL((int32_t)TIMEOUT/3*1000,toMilliseconds(now-beginningOfTimes));
445447
}
446448

449+
// ZOOKEEPER-2894: Memory and completions leak on zookeeper_close
450+
// while there is a request waiting for being processed
451+
// call zookeeper_close() from the main event loop
452+
// assert the completion callback is called
453+
void testCloseWhileInProgressFromMain()
454+
{
455+
Mock_gettimeofday timeMock;
456+
ZookeeperServer zkServer;
457+
CloseFinally guard(&zh);
458+
459+
zh=zookeeper_init("localhost:2121",watcher,10000,TEST_CLIENT_ID,0,0);
460+
CPPUNIT_ASSERT(zh!=0);
461+
forceConnected(zh);
462+
zhandle_t* savezh=zh;
463+
464+
// issue a request
465+
zkServer.addOperationResponse(new ZooGetResponse("1",1));
466+
AsyncGetOperationCompletion res1;
467+
int rc=zoo_aget(zh,"/x/y/1",0,asyncCompletion,&res1);
468+
CPPUNIT_ASSERT_EQUAL((int)ZOK,rc);
469+
470+
// but do not allow Zookeeper C Client to process the request
471+
// and call zookeeper_close() from the main event loop immediately
472+
Mock_free_noop freeMock;
473+
rc=zookeeper_close(zh); zh=0;
474+
freeMock.disable();
475+
CPPUNIT_ASSERT_EQUAL((int)ZOK,rc);
476+
477+
// verify that memory for completions was freed (would be freed if no mock installed)
478+
CPPUNIT_ASSERT_EQUAL(1,freeMock.getFreeCount(savezh));
479+
CPPUNIT_ASSERT(savezh->completions_to_process.head==0);
480+
CPPUNIT_ASSERT(savezh->completions_to_process.last==0);
481+
482+
// verify that completion was called, and it was called with ZCLOSING status
483+
CPPUNIT_ASSERT(res1.called_);
484+
CPPUNIT_ASSERT_EQUAL((int)ZCLOSING,res1.rc_);
485+
}
486+
487+
// ZOOKEEPER-2894: Memory and completions leak on zookeeper_close
488+
// send some request #1
489+
// then, while there is a request #2 waiting for being processed
490+
// call zookeeper_close() from the completion callback of request #1
491+
// assert the completion callback #2 is called
492+
void testCloseWhileInProgressFromCompletion()
493+
{
494+
Mock_gettimeofday timeMock;
495+
ZookeeperServer zkServer;
496+
CloseFinally guard(&zh);
497+
498+
zh=zookeeper_init("localhost:2121",watcher,10000,TEST_CLIENT_ID,0,0);
499+
CPPUNIT_ASSERT(zh!=0);
500+
forceConnected(zh);
501+
zhandle_t* savezh=zh;
502+
503+
// will handle completion on request #1 and issue request #2 from it
504+
class AsyncGetOperationCompletion1: public AsyncCompletion{
505+
public:
506+
AsyncGetOperationCompletion1(zhandle_t **zh, ZookeeperServer *zkServer,
507+
AsyncGetOperationCompletion *res2)
508+
:zh_(zh),zkServer_(zkServer),res2_(res2){}
509+
510+
virtual void dataCompl(int rc1, const char *value, int len, const Stat *stat){
511+
CPPUNIT_ASSERT_EQUAL((int)ZOK,rc1);
512+
513+
// from the completion #1 handler, issue request #2
514+
zkServer_->addOperationResponse(new ZooGetResponse("2",1));
515+
int rc2=zoo_aget(*zh_,"/x/y/2",0,asyncCompletion,res2_);
516+
CPPUNIT_ASSERT_EQUAL((int)ZOK,rc2);
517+
518+
// but do not allow Zookeeper C Client to process the request #2
519+
// and call zookeeper_close() from the completion callback of request #1
520+
rc2=zookeeper_close(*zh_); *zh_=0;
521+
CPPUNIT_ASSERT_EQUAL((int)ZOK,rc2);
522+
523+
// do not disable freeMock here, let completion #2 handler
524+
// return through ZooKeeper C Client internals to the main loop
525+
// and fulfill the work
526+
}
527+
528+
zhandle_t **zh_;
529+
ZookeeperServer *zkServer_;
530+
AsyncGetOperationCompletion *res2_;
531+
};
532+
533+
// issue request #1
534+
AsyncGetOperationCompletion res2;
535+
AsyncGetOperationCompletion1 res1(&zh,&zkServer,&res2);
536+
zkServer.addOperationResponse(new ZooGetResponse("1",1));
537+
int rc=zoo_aget(zh,"/x/y/1",0,asyncCompletion,&res1);
538+
CPPUNIT_ASSERT_EQUAL((int)ZOK,rc);
539+
540+
// process the send queue
541+
int fd; int interest; timeval tv;
542+
rc=zookeeper_interest(zh,&fd,&interest,&tv);
543+
CPPUNIT_ASSERT_EQUAL((int)ZOK,rc);
544+
CPPUNIT_ASSERT(zh!=0);
545+
Mock_free_noop freeMock;
546+
while(zh!=0 && (rc=zookeeper_process(zh,interest))==ZOK) {
547+
millisleep(100);
548+
}
549+
freeMock.disable();
550+
CPPUNIT_ASSERT(zh==0);
551+
552+
// verify that memory for completions was freed (would be freed if no mock installed)
553+
CPPUNIT_ASSERT_EQUAL(1,freeMock.getFreeCount(savezh));
554+
CPPUNIT_ASSERT(savezh->completions_to_process.head==0);
555+
CPPUNIT_ASSERT(savezh->completions_to_process.last==0);
556+
557+
// verify that completion #2 was called, and it was called with ZCLOSING status
558+
CPPUNIT_ASSERT(res2.called_);
559+
CPPUNIT_ASSERT_EQUAL((int)ZCLOSING,res2.rc_);
560+
}
561+
447562
#else
448563
class TestGetDataJob: public TestJob{
449564
public:

0 commit comments

Comments
 (0)