Fix: memory leak in transaction handles (#410)

- New handles are no longer allocated when starting new transactions.
- Extend transaction testcase

connection.go

@@ -77,9 +77,11 @@ func (conn *Conn) Close() error {
 
 	C.OCIHandleFree(unsafe.Pointer(conn.svc), C.OCI_HTYPE_SVCCTX)
 	C.OCIHandleFree(unsafe.Pointer(conn.errHandle), C.OCI_HTYPE_ERROR)
+	C.OCIHandleFree(unsafe.Pointer(conn.txHandle), C.OCI_HTYPE_TRANS)
 	C.OCIHandleFree(unsafe.Pointer(conn.env), C.OCI_HTYPE_ENV)
 	conn.svc = nil
 	conn.errHandle = nil
+	conn.txHandle = nil
 	conn.env = nil
 
 	return err
@@ -157,32 +159,14 @@ func (conn *Conn) BeginTx(ctx context.Context, txOptions driver.TxOptions) (driv
 	}
 
 	if conn.transactionMode != C.OCI_TRANS_READWRITE {
-		// transaction handle
-		trans, _, err := conn.ociHandleAlloc(C.OCI_HTYPE_TRANS, 0)
-		if err != nil {
-			return nil, fmt.Errorf("allocate transaction handle error: %v", err)
-		}
-
-		// sets the transaction context attribute of the service context
-		err = conn.ociAttrSet(unsafe.Pointer(conn.svc), C.OCI_HTYPE_SVCCTX, *trans, 0, C.OCI_ATTR_TRANS)
-		if err != nil {
-			C.OCIHandleFree(*trans, C.OCI_HTYPE_TRANS)
-			return nil, err
-		}
-
-		// transaction handle should be freed by something once attached to the service context
-		// but I cannot find anything in the documentation explicitly calling this out
-		// going by examples: https://docs.oracle.com/cd/B28359_01/appdev.111/b28395/oci17msc006.htm#i428845
-
 		if rv := C.OCITransStart(
 			conn.svc,
 			conn.errHandle,
 			0,
-			conn.transactionMode, // mode is: C.OCI_TRANS_SERIALIZABLE, C.OCI_TRANS_READWRITE, or C.OCI_TRANS_READONLY
+			conn.transactionMode|C.OCI_TRANS_NEW, // mode is: C.OCI_TRANS_SERIALIZABLE, C.OCI_TRANS_READWRITE, or C.OCI_TRANS_READONLY
 		); rv != C.OCI_SUCCESS {
 			return nil, conn.getError(rv)
 		}
-
 	}
 
 	conn.inTransaction = true

globals.go

@@ -63,6 +63,7 @@ type (
 		env                  *C.OCIEnv
 		errHandle            *C.OCIError
 		usrSession           *C.OCISession
+		txHandle             *C.OCITrans
 		prefetchRows         C.ub4
 		prefetchMemory       C.ub4
 		transactionMode      C.ub4

oci8.go

@@ -229,6 +229,10 @@ func (drv *DriverStruct) Open(dsnString string) (driver.Conn, error) {
 					C.OCI_DEFAULT,
 				)
 			}
+			if conn.txHandle != nil {
+				C.OCIHandleFree(unsafe.Pointer(conn.txHandle), C.OCI_HTYPE_TRANS)
+				conn.txHandle = nil
+			}
 			if conn.usrSession != nil {
 				C.OCIHandleFree(unsafe.Pointer(conn.usrSession), C.OCI_HTYPE_SESSION)
 				conn.usrSession = nil
@@ -389,7 +393,19 @@ func (drv *DriverStruct) Open(dsnString string) (driver.Conn, error) {
 		}
 		conn.svc = *svcCtxPP
 		doneLogon = true
+	}
+
+	// Create transaction context.
+	handle, _, err = conn.ociHandleAlloc(C.OCI_HTYPE_TRANS, 0)
+	if err != nil {
+		return nil, fmt.Errorf("allocate transaction handle error: %v", err)
+	}
+	conn.txHandle = (*C.OCITrans)(*handle)
 
+	// Set transaction context attribute of the service context.
+	err = conn.ociAttrSet(unsafe.Pointer(conn.svc), C.OCI_HTYPE_SVCCTX, *handle, 0, C.OCI_ATTR_TRANS)
+	if err != nil {
+		return nil, fmt.Errorf("service context attribute set error: %v", err)
 	}
 
 	conn.transactionMode = dsn.transactionMode

oci8_sql_test.go

@@ -781,6 +781,84 @@ func TestDestructiveTransaction(t *testing.T) {
 		},
 	}
 	testRunQueryResults(t, queryResults)
+
+	// Run new transactions after the others finished successfully.
+	var tx3 *sql.Tx
+	var tx4 *sql.Tx
+
+	tx3, err = TestDB.BeginTx(ctx, nil)
+	if err != nil {
+		t.Fatal("begin tx error:", err)
+	}
+
+	tx4, err = TestDB.BeginTx(ctx, nil)
+	if err != nil {
+		t.Fatal("begin tx error:", err)
+	}
+
+	result, err = tx4.ExecContext(ctx, "update TRANSACTION_"+TestTimeString+" set B = :1 where A = :2", []interface{}{99, 1}...)
+	if err != nil {
+		t.Fatal("exec error:", err)
+	}
+
+	count, err = result.RowsAffected()
+	if err != nil {
+		t.Fatal("rows affected error:", err)
+	}
+	if count != 1 {
+		t.Fatalf("rows affected %v not equal to 1", count)
+	}
+
+	result, err = tx3.ExecContext(ctx, "update TRANSACTION_"+TestTimeString+" set B = :1 where A = :2", []interface{}{88, 6}...)
+	if err != nil {
+		t.Fatal("exec error:", err)
+	}
+
+	count, err = result.RowsAffected()
+	if err != nil {
+		t.Fatal("rows affected error:", err)
+	}
+	if count != 1 {
+		t.Fatalf("rows affected %v not equal to 1", count)
+	}
+
+	queryResults = testQueryResults{
+		query: "select A, B, C from TRANSACTION_" + TestTimeString + " order by A",
+		queryResults: []testQueryResult{
+			{
+				results: [][]interface{}{
+					{int64(1), int64(22), int64(3)},
+					{int64(4), int64(55), int64(6)},
+					{int64(6), int64(7), int64(8)},
+				},
+			},
+		},
+	}
+	testRunQueryResults(t, queryResults)
+
+	err = tx3.Commit()
+	if err != nil {
+		t.Fatal("commit err:", err)
+	}
+
+	err = tx4.Rollback()
+	if err != nil {
+		t.Fatal("commit err:", err)
+	}
+
+	queryResults = testQueryResults{
+		query: "select A, B, C from TRANSACTION_" + TestTimeString + " order by A",
+		queryResults: []testQueryResult{
+			{
+				results: [][]interface{}{
+					{int64(1), int64(22), int64(3)},
+					{int64(4), int64(55), int64(6)},
+					{int64(6), int64(88), int64(8)},
+				},
+			},
+		},
+	}
+	testRunQueryResults(t, queryResults)
 }
 
 // TestSelectDualNull checks null from dual