From 9d70992a1d3892ee3fd291dc97c7f597ec77bf77 Mon Sep 17 00:00:00 2001 From: "Bradley C. Kuszmaul" Date: Fri, 21 Dec 2007 19:59:31 +0000 Subject: [PATCH] Clean up the valgrind memory leaks (caused by lots of subtle c++ bugs. Addresse #215 git-svn-id: file:///svn/tokudb@1320 c7de825b-a66e-492c-adef-691d508d4ae1 --- cxx/Makefile | 4 ++++ cxx/db.cpp | 23 +++++++++++++++++++---- cxx/dbc.cpp | 2 +- cxx/dbenv.cpp | 23 ++++++++++++++++++----- cxx/tests/Makefile | 14 ++++++++++++-- cxx/tests/test1.cpp | 3 ++- cxx/tests/test1e.cpp | 1 - cxx/txn.cpp | 3 ++- include/db.h | 3 ++- include/db_cxx.h | 4 ++++ 10 files changed, 64 insertions(+), 16 deletions(-) diff --git a/cxx/Makefile b/cxx/Makefile index ee0bdb22b63..ec50e22b8cf 100644 --- a/cxx/Makefile +++ b/cxx/Makefile @@ -15,3 +15,7 @@ test1: test1.o dbt.o db.o dbenv.o ../lib/libdb.a $(LIBNAME).a: $(OBJS) $(AR) rv $@ $(OBJS) +clean: + rm -f $(OBJS) $(LIBNAME).a $(LIBNAME).so + + diff --git a/cxx/db.cpp b/cxx/db.cpp index 5de180b3c33..a393afcbfe4 100644 --- a/cxx/db.cpp +++ b/cxx/db.cpp @@ -27,11 +27,13 @@ Db::Db(DbEnv *env, u_int32_t flags) } Db::~Db() { - if (is_private_env) { - delete the_Env; // The destructor closes the env. + if (is_private_env && the_Env) { + the_Env->close(0); + delete the_Env; } - if (!the_db) { + if (the_db) { close(0); // the user should have called close, but we do it here if not done. + assert(the_db==0); } } @@ -41,13 +43,25 @@ int Db::close (u_int32_t flags) { } the_db->api_internal = 0; + int ret = the_db->close(the_db, flags); the_db = 0; + + int no_exceptions = the_Env->do_no_exceptions; // Get this out before possibly deleting the env + + if (is_private_env) { + // The env was closed by the_db->close, we have to tell the DbEnv that the DB_ENV is gone, and delete it. + the_Env->the_env = 0; + delete the_Env; + the_Env=0; + } + // Do we need to clean up "private environments"? // What about cursors? They should be cleaned up already, but who did it? - return the_Env->maybe_throw_error(ret); + // This maybe_throw must be the static one because the env is gone. + return DbEnv::maybe_throw_error(ret, NULL, no_exceptions); } int Db::open(DbTxn *txn, const char *filename, const char *subname, DBTYPE typ, u_int32_t flags, int mode) { @@ -72,6 +86,7 @@ int Db::set_pagesize(u_int32_t size) { int Db::remove(const char *file, const char *database, u_int32_t flags) { int ret = the_db->remove(the_db, file, database, flags); + the_db = 0; return the_Env->maybe_throw_error(ret); } diff --git a/cxx/dbc.cpp b/cxx/dbc.cpp index b2bff97b949..d2da805fd7d 100644 --- a/cxx/dbc.cpp +++ b/cxx/dbc.cpp @@ -2,8 +2,8 @@ int Dbc::close (void) { DBC *dbc = this; + DbEnv *env = (DbEnv*)dbc->dbp->api_internal; // Must grab the env before closing the cursor. int ret = dbc->c_close(dbc); - DbEnv *env = (DbEnv*)dbc->dbp->api_internal; return env->maybe_throw_error(ret); } diff --git a/cxx/dbenv.cpp b/cxx/dbenv.cpp index 1fad1e42564..6bf9f7dfbfb 100644 --- a/cxx/dbenv.cpp +++ b/cxx/dbenv.cpp @@ -23,9 +23,18 @@ DbEnv::DbEnv(DB_ENV *env, u_int32_t flags) the_env->api1_internal = this; } +// If still open, close it. In most cases, the caller should call close explicitly so that they can catch the exceptions. +DbEnv::~DbEnv(void) +{ + if (the_env!=NULL) { + (void)the_env->close(the_env, 0); + the_env = 0; + } +} + int DbEnv::close(u_int32_t flags) { int ret = the_env->close(the_env, flags); - the_env = 0; /* get rid of the env ref, so we don't touch it (even if we failed.) */ + the_env = 0; /* get rid of the env ref, so we don't touch it (even if we failed, or when the destructor is called) */ return maybe_throw_error(ret); } @@ -64,19 +73,23 @@ void DbEnv::set_errpfx(const char *errpfx) { the_env->set_errpfx(the_env, errpfx); } -int DbEnv::maybe_throw_error(int err) throw (DbException) { +int DbEnv::maybe_throw_error(int err, DbEnv *env, int no_exceptions) throw (DbException) { if (err==0) return 0; - if (do_no_exceptions) return err; + if (no_exceptions) return err; if (err==DB_LOCK_DEADLOCK) { - DbDeadlockException e(this); + DbDeadlockException e(env); throw e; } else { DbException e(err); - e.set_env(this); + e.set_env(env); throw e; } } +int DbEnv::maybe_throw_error(int err) throw (DbException) { + return maybe_throw_error(err, this, do_no_exceptions); +} + extern "C" { void toku_db_env_err_vararg(const DB_ENV * env, int error, const char *fmt, va_list ap); }; diff --git a/cxx/tests/Makefile b/cxx/tests/Makefile index e577838b86a..9599a386b83 100644 --- a/cxx/tests/Makefile +++ b/cxx/tests/Makefile @@ -5,6 +5,12 @@ CPPFLAGS = -I../ -I../../include CXXFLAGS = -Wall -g LDLIBS = ../../lib/libtdb_cxx.a ../../lib/libdb.a -lz +ifeq ($(OSX),OSX) + VGRIND= +else + VGRIND=valgrind --quiet --error-exitcode=1 --leak-check=yes +endif + all: $(TARGETS) $(TARGETS): $(DBCXX) @@ -15,5 +21,9 @@ clean: rm -rf $(TARGETS) check: $(TARGETS) - ./test1 - ./test1e + $(VGRIND) ./test1 + $(VGRIND) ./test1e + $(VGRIND) ./db_create foo.db a b c d + $(VGRIND) ./db_dump foo.db > foo.out + (echo " 61";echo " 62";echo " 63";echo " 64")>foo.expectout + diff foo.out foo.expectout diff --git a/cxx/tests/test1.cpp b/cxx/tests/test1.cpp index e93f83b5759..1af30fca874 100644 --- a/cxx/tests/test1.cpp +++ b/cxx/tests/test1.cpp @@ -36,6 +36,8 @@ void test_db(void) { r = db.set_bt_compare(cmp); assert(r == 0); r = db.remove("DoesNotExist.db", NULL, 0); assert(r == ENOENT); + // The db is closed + r = env.close(0); assert(r== 0); } void test_db_env(void) { @@ -55,6 +57,5 @@ int main() test_dbt(); test_db(); test_db_env(); - cout << "Hello World!" << endl; cout << "Welcome to C++ Programming" << endl; return 0; } diff --git a/cxx/tests/test1e.cpp b/cxx/tests/test1e.cpp index 9d13b29528b..f2e849c283f 100644 --- a/cxx/tests/test1e.cpp +++ b/cxx/tests/test1e.cpp @@ -68,6 +68,5 @@ int main() test_dbt(); test_db(); test_db_env(); - cout << "Hello World!" << endl; cout << "Welcome to C++ Programming" << endl; return 0; } diff --git a/cxx/txn.cpp b/cxx/txn.cpp index 8b0087967bb..27444c72681 100644 --- a/cxx/txn.cpp +++ b/cxx/txn.cpp @@ -9,5 +9,6 @@ DbTxn::DbTxn(DB_TXN *txn) int DbTxn::commit (u_int32_t flags) { DB_TXN *txn = get_DB_TXN(); int ret = txn->commit(txn, flags); - return ret; + DbEnv *env = (DbEnv*)txn->mgrp->api1_internal; + return env->maybe_throw_error(ret); } diff --git a/include/db.h b/include/db.h index 5e3bc8f1fcd..dcd79ae0834 100644 --- a/include/db.h +++ b/include/db.h @@ -211,8 +211,9 @@ struct __toku_db_txn_active { char __toku_dummy2[184]; /* Padding at the end */ }; struct __toku_db_txn { + DB_ENV *mgrp /*In TokuDB, mgrp is a DB_ENV not a DB_TXNMGR*/; /* 32-bit offset=0 size=4, 64=bit offset=0 size=8 */ struct __toku_db_txn_internal *i; - void* __toku_dummy0[18]; + void* __toku_dummy0[17]; char __toku_dummy1[8]; void *api_internal; /* 32-bit offset=84 size=4, 64=bit offset=160 size=8 */ void* __toku_dummy2[2]; diff --git a/include/db_cxx.h b/include/db_cxx.h index 67846513608..b0430471b45 100644 --- a/include/db_cxx.h +++ b/include/db_cxx.h @@ -117,8 +117,10 @@ class Db { class DbEnv { friend class Db; friend class Dbc; + friend class DbTxn; public: DbEnv(u_int32_t flags); + ~DbEnv(void); DB_ENV *get_DB_ENV(void) { if (this==0) return 0; @@ -145,6 +147,7 @@ class DbEnv { DbEnv(DB_ENV *, u_int32_t /*flags*/); int maybe_throw_error(int /*err*/) throw (DbException); + static int maybe_throw_error(int, DbEnv*, int /*no_exceptions*/) throw (DbException); }; @@ -161,6 +164,7 @@ class DbTxn { DbTxn(DB_TXN*); private: DB_TXN *the_txn; + }; class Dbc : protected DBC