From 8ba3c107ffad11fb5fd751c9cd2b47305dd66ad3 Mon Sep 17 00:00:00 2001
From: Vaidas Gasiunas <vaidas.gasiunas@snowflake.com>
Date: Sat, 12 Mar 2022 14:12:37 +0100
Subject: [PATCH] ApiTester: cancel transaction instead of deleting to avoid
 crashes when it is accessed by pending callbacks

---
 bindings/c/test/apitester/TesterApiWrapper.cpp  | 17 +++++++++++++++++
 bindings/c/test/apitester/TesterApiWrapper.h    |  1 +
 .../apitester/TesterTransactionExecutor.cpp     |  4 +++-
 bindings/c/test/apitester/TesterWorkload.cpp    |  4 ++--
 4 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/bindings/c/test/apitester/TesterApiWrapper.cpp b/bindings/c/test/apitester/TesterApiWrapper.cpp
index 869b02d89c..4fc3b79c9a 100644
--- a/bindings/c/test/apitester/TesterApiWrapper.cpp
+++ b/bindings/c/test/apitester/TesterApiWrapper.cpp
@@ -18,6 +18,7 @@
  * limitations under the License.
  */
 #include "TesterApiWrapper.h"
+#include "TesterUtil.h"
 #include <cstdint>
 #include <fmt/format.h>
 
@@ -41,14 +42,17 @@ void Future::reset() {
 }
 
 void Future::cancel() {
+	ASSERT(future_);
 	fdb_future_cancel(future_.get());
 }
 
 fdb_error_t Future::getError() const {
+	ASSERT(future_);
 	return fdb_future_get_error(future_.get());
 }
 
 std::optional<std::string> ValueFuture::getValue() const {
+	ASSERT(future_);
 	int out_present;
 	const std::uint8_t* val;
 	int vallen;
@@ -60,35 +64,48 @@ std::optional<std::string> ValueFuture::getValue() const {
 Transaction::Transaction(FDBTransaction* tx) : tx_(tx, fdb_transaction_destroy) {}
 
 ValueFuture Transaction::get(std::string_view key, fdb_bool_t snapshot) {
+	ASSERT(tx_);
 	return ValueFuture(fdb_transaction_get(tx_.get(), (const uint8_t*)key.data(), key.size(), snapshot));
 }
 
 void Transaction::set(std::string_view key, std::string_view value) {
+	ASSERT(tx_);
 	fdb_transaction_set(tx_.get(), (const uint8_t*)key.data(), key.size(), (const uint8_t*)value.data(), value.size());
 }
 
 void Transaction::clear(std::string_view key) {
+	ASSERT(tx_);
 	fdb_transaction_clear(tx_.get(), (const uint8_t*)key.data(), key.size());
 }
 
 void Transaction::clearRange(std::string_view begin, std::string_view end) {
+	ASSERT(tx_);
 	fdb_transaction_clear_range(
 	    tx_.get(), (const uint8_t*)begin.data(), begin.size(), (const uint8_t*)end.data(), end.size());
 }
 
 Future Transaction::commit() {
+	ASSERT(tx_);
 	return Future(fdb_transaction_commit(tx_.get()));
 }
 
+void Transaction::cancel() {
+	ASSERT(tx_);
+	fdb_transaction_cancel(tx_.get());
+}
+
 Future Transaction::onError(fdb_error_t err) {
+	ASSERT(tx_);
 	return Future(fdb_transaction_on_error(tx_.get(), err));
 }
 
 void Transaction::reset() {
+	ASSERT(tx_);
 	fdb_transaction_reset(tx_.get());
 }
 
 fdb_error_t Transaction::setOption(FDBTransactionOption option) {
+	ASSERT(tx_);
 	return fdb_transaction_set_option(tx_.get(), option, reinterpret_cast<const uint8_t*>(""), 0);
 }
 
diff --git a/bindings/c/test/apitester/TesterApiWrapper.h b/bindings/c/test/apitester/TesterApiWrapper.h
index 61275f1109..567d9348c1 100644
--- a/bindings/c/test/apitester/TesterApiWrapper.h
+++ b/bindings/c/test/apitester/TesterApiWrapper.h
@@ -71,6 +71,7 @@ public:
 	void clear(std::string_view key);
 	void clearRange(std::string_view begin, std::string_view end);
 	Future commit();
+	void cancel();
 	Future onError(fdb_error_t err);
 	void reset();
 	fdb_error_t setOption(FDBTransactionOption option);
diff --git a/bindings/c/test/apitester/TesterTransactionExecutor.cpp b/bindings/c/test/apitester/TesterTransactionExecutor.cpp
index c50a6fd198..563350acb9 100644
--- a/bindings/c/test/apitester/TesterTransactionExecutor.cpp
+++ b/bindings/c/test/apitester/TesterTransactionExecutor.cpp
@@ -87,6 +87,9 @@ public:
 		}
 		txState = TxState::DONE;
 		lock.unlock();
+		// cancel transaction so that any pending operations on it
+		// fail gracefully
+		fdbTx.cancel();
 		txActor->complete(error_code_success);
 		cleanUp();
 		contAfterDone();
@@ -102,7 +105,6 @@ protected:
 		ASSERT(txState == TxState::DONE);
 		ASSERT(!onErrorFuture);
 		txActor = {};
-		fdbTx = {};
 	}
 
 	// Complete the transaction with an (unretriable) error
diff --git a/bindings/c/test/apitester/TesterWorkload.cpp b/bindings/c/test/apitester/TesterWorkload.cpp
index ce269ca824..19b28731e9 100644
--- a/bindings/c/test/apitester/TesterWorkload.cpp
+++ b/bindings/c/test/apitester/TesterWorkload.cpp
@@ -145,9 +145,9 @@ void WorkloadManager::run() {
 	}
 	scheduler->join();
 	if (failed()) {
-		fmt::print(stderr, "{} workloads failed", numWorkloadsFailed);
+		fmt::print(stderr, "{} workloads failed\n", numWorkloadsFailed);
 	} else {
-		fprintf(stderr, "All workloads succesfully completed");
+		fprintf(stderr, "All workloads succesfully completed\n");
 	}
 }