Merge pull request #29 from wojas/cgo-txn-new-fix

Fix: allocate C memory for MDB_val in readonly Txn

Author: wojas

lmdb/env.go

@@ -113,13 +113,13 @@ func (env *Env) FD() (uintptr, error) {
 	// to avoid constant value overflow errors at compile time.
 	const fdInvalid = ^uintptr(0)
 
-	mf := new(C.mdb_filehandle_t)
-	ret := C.mdb_env_get_fd(env._env, mf)
+	var mf C.mdb_filehandle_t
+	ret := C.mdb_env_get_fd(env._env, &mf)
 	err := operrno("mdb_env_get_fd", ret)
 	if err != nil {
 		return 0, err
 	}
-	fd := uintptr(*mf)
+	fd := uintptr(mf)
 
 	if fd == fdInvalid {
 		return 0, errNotOpen

lmdb/txn.go

@@ -63,6 +63,9 @@ type Txn struct {
 	// reset/renewed
 	id uintptr
 
+	// Pointer to scratch space for key and val in readonly transactions
+	cbuf unsafe.Pointer
+
 	env  *Env
 	_txn *C.MDB_txn
 	key  *C.MDB_val
@@ -87,12 +90,20 @@ func beginTxn(env *Env, parent *Txn, flags uint) (*Txn, error) {
 			txn.key = env.ckey
 			txn.val = env.cval
 		} else {
-			// It is not easy to share C.MDB_val values in this scenario unless
-			// there is a synchronized pool involved, which will increase
-			// overhead.  Further, allocating these values with C will add
-			// overhead both here and when the values are freed.
-			txn.key = new(C.MDB_val)
-			txn.val = new(C.MDB_val)
+			// Readonly transaction.
+			// We cannot share global C.MDB_val values in this scenario, because
+			// there can be multiple simultaneous read transactions.
+			// Allocate C memory for two values in one call.
+			// This is freed in clearTxn(), which is always called at the end
+			// of a transaction through Commit() or Abort().
+			if C.sizeof_MDB_val == 0 {
+				panic("zero C.sizeof_MDB_val") // should never happen
+			}
+			txn.cbuf = C.malloc(2 * C.sizeof_MDB_val)
+			txn.key = (*C.MDB_val)(txn.cbuf)
+			txn.val = (*C.MDB_val)(unsafe.Pointer(
+				uintptr(txn.cbuf) + uintptr(C.sizeof_MDB_val),
+			))
 		}
 	} else {
 		// Because parent Txn objects cannot be used while a sub-Txn is active
@@ -240,6 +251,14 @@ func (txn *Txn) clearTxn() {
 	// sure the value returned for an invalid Txn is more or less consistent
 	// for people familiar with the C semantics.
 	txn.resetID()
+
+	// Release C allocated buffer, if used
+	if txn.cbuf != nil {
+		txn.key = nil
+		txn.val = nil
+		C.free(txn.cbuf)
+		txn.cbuf = nil
+	}
 }
 
 // resetID has to be called anytime the value of Txn.getID() may change

lmdb/txn_test.go

@@ -918,6 +918,69 @@ func TestTxn_Reset_writeTxn(t *testing.T) {
 	}
 }
 
+// This test demonstrates that in a readonly transaction C memory is allocated
+// for the values, and freed during a Reset.
+func TestTxn_Reset_readonly_C_free(t *testing.T) {
+	env := setup(t)
+	path, err := env.Path()
+	if err != nil {
+		env.Close()
+		t.Error(err)
+		return
+	}
+	defer os.RemoveAll(path)
+	defer env.Close()
+
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+
+	txn, err := env.BeginTxn(nil, Readonly)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	defer txn.Abort()
+
+	// Since this is a readonly transaction, the global Env key/val cannot be
+	// reused and new C memory must be allocated.
+	if txn.key == env.ckey || txn.val == env.cval {
+		t.Error("env.ckey and cval must not be used in a readonly Txn")
+	}
+	if txn.cbuf == nil {
+		t.Error("cbuf expected to not be nil when opening a readonly Txn")
+	}
+
+	// Reset must not free the buffer
+	txn.Reset()
+	if txn.cbuf == nil {
+		t.Error("cbuf must not be nil after Reset")
+		return
+	}
+
+	// Renew must not free the buffer
+	err = txn.Renew()
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	if txn.cbuf == nil {
+		t.Error("cbuf must not be nil after Renew")
+		return
+	}
+
+	// Abort must free the buffer
+	txn.Abort()
+	if txn.cbuf != nil {
+		t.Error("cbuf expected to be nil, C memory not freed")
+	}
+	if txn.key != nil || txn.val != nil {
+		t.Error("key and val expected to be nil after C memory free")
+	}
+
+	// A second Abort must not panic
+	txn.Abort()
+}
+
 func TestTxn_UpdateLocked(t *testing.T) {
 	env := setup(t)
 	path, err := env.Path()