Adding exception type for empty callback functions (reopening of PR #301)

.github/workflows/ci.yml

@@ -436,6 +436,7 @@ jobs:
           tidy-checks: '' # Read .clang-tidy for configuration
           database: build/Release/compile_commands.json
           version: 12
+
 
       - name: Run linters on tests
         continue-on-error: true
@@ -451,6 +452,7 @@ jobs:
           database: build/Release/compile_commands.json
           version: 12
 
+
       - name: Report lint failure
         if: steps.source-linter.outputs.checks-failed > 0 || steps.test-linter.outputs.checks-failed > 0
         run: |

include/up-cpp/utils/CallbackConnection.h

@@ -193,7 +193,7 @@
 	};
 
 	/// @brief Semi-private constructor. Use the static establish() instead.
 	Connection(std::shared_ptr<Callback> cb, PrivateConstructToken)
 	    : callback_(cb) {}
 
 	// Connection is only ever available wrapped in a std::shared_ptr.
@@ -230,10 +230,22 @@
 /// reason.
 struct BadConnection : public std::runtime_error {
 	template <typename... Args>
 	BadConnection(Args&&... args)
 	    : std::runtime_error(std::forward<Args>(args)...) {}
 };
 
+/// @brief Thrown if an empty std::function parameter was received
+///
+/// A std::function can be empty. When an empty function is invoked, it will
+/// throw std::bad_function_call. We can check earlier by casting the function
+/// to a boolean. If the check fails, EmptyFunctionObject is thrown. This makes
+/// the error appear earlier without waiting for invocation to occur.
+struct EmptyFunctionObject : public std::invalid_argument {
+	template <typename... Args>
+	EmptyFunctionObject(Args&&... args)
+	    : std::invalid_argument(std::forward<Args>(args)...) {}
+};
+
 template <typename RT, typename... Args>
 struct [[nodiscard]] CalleeHandle {
 	using Conn = Connection<RT, Args...>;
@@ -245,7 +257,7 @@
 	CalleeHandle(std::shared_ptr<Conn> connection,
 	             std::shared_ptr<typename Conn::Callback> callback,
 	             std::optional<typename Conn::Cleanup>&& cleanup,
 	             typename Conn::PrivateConstructToken)
 	    : connection_(connection),
 	      callback_(callback),
 	      cleanup_(std::move(cleanup)) {
@@ -254,11 +266,21 @@
 			    "Attempted to create a connected CalleeHandle with bad "
 			    "connection pointer");
 		}
+
 		if (!callback_) {
 			throw BadConnection(
 			    "Attempted to create a connected CalleeHandle with bad "
 			    "callback pointer");
 		}
+
+		const auto& callback_obj = *callback_;
+		if (!callback_obj) {
+			throw EmptyFunctionObject("Callback function is empty");
+		}
+
+		if (cleanup_ && !cleanup_.value()) {
+			throw EmptyFunctionObject("Cleanup function is empty");
+		}
 	}
 
 	/// @brief CalleeHandles can be move constructed
@@ -311,7 +333,7 @@
 	///     * False if the connection has been broken (i.e. This handle has
 	///       been reset/moved, or all other references to the connection
 	///       have been discarded)
 	bool isConnected() const {
 		auto locked_connection = connection_.lock();
 		return locked_connection && (*locked_connection);
 	}
@@ -329,7 +351,7 @@
 /// CallerHandle that needs to be corrected.
 struct BadCallerAccess : public std::logic_error {
 	template <typename... Args>
 	BadCallerAccess(Args&&... args)
 	    : std::logic_error(std::forward<Args>(args)...) {}
 };
 
@@ -342,7 +364,7 @@
 
 	/// @brief Creates a connected handle. Only usable by Connection
 	CallerHandle(std::shared_ptr<Conn> connection,
 	             typename Conn::PrivateConstructToken)
 	    : connection_(connection) {
 		if (!connection_) {
 			throw BadConnection(
@@ -368,7 +390,7 @@
 	///     * False if the connection has been broken (i.e. This handle has
 	///       been reset/moved, or all other references to the connection
 	///       have been discarded)
 	bool isConnected() const { return connection_ && (*connection_); }
 
 	/// @throws BadCallerAccess if this handle has been default constructed OR
 	///         reset() has left it without a valid conneciton pointer.
@@ -418,7 +440,7 @@
 		value_ = std::move(v);
 		return *this;
 	}
 	operator std::optional<RT>&&() && { return std::move(value_); }
 
 private:
 	std::optional<RT> value_;

test/coverage/communication/NotificationSinkTest.cpp

@@ -21,7 +21,7 @@
 
 namespace {
 using MsgDiff = google::protobuf::util::MessageDifferencer;
 using namespace uprotocol::communication;
 namespace UriValidator = uprotocol::datamodel::validator::uri;
 
 class NotificationSinkTest : public testing::Test {
@@ -96,7 +96,7 @@
 		    "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 		    "abcdefghijklmnopqrstuvwxyz";
 		const size_t max_index = (sizeof(charset) - 1);
 		return charset[rand() % max_index];
 	};
 	std::string str(length, 0);
 	std::generate_n(str.begin(), length, randchar);
@@ -216,14 +216,23 @@
 	    testDefaultSourceUUri_);
 
 	// bind to null callback
-	auto result = NotificationSink::create(transport, transport->getEntityUri(),
-	                                       std::move(nullptr), testTopicUUri_);
+	auto test_create_nullptr = [transport, this]() {
+		std::ignore =
+		    NotificationSink::create(transport, transport->getEntityUri(),
+		                             std::move(nullptr), testTopicUUri_);
+	};
+
+	using namespace uprotocol::utils;
+
+	EXPECT_THROW(test_create_nullptr(), callbacks::EmptyFunctionObject);
+
+	// Default construct a function object
+	auto test_create_empty = [transport, this]() {
+		std::ignore = NotificationSink::create(
+		    transport, transport->getEntityUri(), {}, testTopicUUri_);
+	};
 
-	uprotocol::v1::UMessage msg;
-	auto attr = std::make_shared<uprotocol::v1::UAttributes>();
-	*msg.mutable_attributes() = *attr;
-	msg.set_payload(get_random_string(1400));
-	EXPECT_THROW(transport->mockMessage(msg), std::bad_function_call);
+	EXPECT_THROW(test_create_empty(), callbacks::EmptyFunctionObject);
 }
 
 }  // namespace

test/coverage/communication/SubscriberTest.cpp

@@ -90,7 +90,7 @@
 		    "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 		    "abcdefghijklmnopqrstuvwxyz";
 		const size_t max_index = (sizeof(charset) - 1);
 		return charset[rand() % max_index];
 	};
 	std::string str(length, 0);
 	std::generate_n(str.begin(), length, randchar);
@@ -186,14 +186,21 @@
 	    testDefaultSourceUUri_);
 
 	// bind to null callback
-	auto result =
-	    Subscriber::subscribe(transport, testTopicUUri_, std::move(nullptr));
+	auto test_subscribe_nullptr = [transport, this]() {
+		std::ignore = Subscriber::subscribe(transport, testTopicUUri_,
+		                                    std::move(nullptr));
+	};
+
+	using namespace uprotocol::utils;
+
+	EXPECT_THROW(test_subscribe_nullptr(), callbacks::EmptyFunctionObject);
+
+	// Default construct a function object
+	auto test_subscribe_empty = [transport, this]() {
+		std::ignore = Subscriber::subscribe(transport, testTopicUUri_, {});
+	};
 
-	uprotocol::v1::UMessage msg;
-	auto attr = std::make_shared<uprotocol::v1::UAttributes>();
-	*msg.mutable_attributes() = *attr;
-	msg.set_payload(get_random_string(1400));
-	EXPECT_THROW(transport->mockMessage(msg), std::bad_function_call);
+	EXPECT_THROW(test_subscribe_empty(), callbacks::EmptyFunctionObject);
 }
 
 }  // namespace

test/coverage/utils/CallbackConnectionTest.cpp

@@ -586,11 +586,11 @@
 	auto [handle, callable] = callbacks::Connection<bool>::establish(
 	    [&fake_blocking_op]() { return fake_blocking_op.try_acquire_for(1s); });
 
 	auto caller_fn = ([callable, &callbacks_pending, &callbacks_released,
 	                   &main_task_sync]() mutable {
 		++callbacks_pending;
 		main_task_sync.release();
 		auto did_not_expire = callable();
 		--callbacks_pending;
 		if (did_not_expire) {
 			++callbacks_released;
@@ -698,4 +698,73 @@
 	});
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// It is possible to create std::function objects with no target function. When
+// they are invoked, they throw std::bad_function_call. This is not desireable,
+// so the callback connections modules are required to check the validity of
+// function objects they receive
+
+// Tests invalid callback function objects
+TEST_F(CallbackTest, EstablishWithNonCallableCallback) {
+	using namespace uprotocol::utils;
+
+	callbacks::Connection<bool>::ConnectedPair conn;
+
+	EXPECT_THROW(conn = callbacks::Connection<bool>::establish({}),
+	             callbacks::EmptyFunctionObject);
+
+	auto& [handle, callable] = conn;
+
+	// Ordering is important here. If handle.reset() tries blindly to call the
+	// cleanup callback, the exception could be thrown before the connection
+	// is broken. When that happens, the destructor will try to reset again.
+	// By resetting the callable second, there is no need to try the cleanup
+	// funciton again, so the destructor won't throw.
+	EXPECT_NO_THROW(handle.reset());
+	EXPECT_NO_THROW(callable.reset());
+}
+
+// Tests invalid cleanup function objects
+TEST_F(CallbackTest, EstablishWithNonCallableCleanup) {
+	using namespace uprotocol::utils;
+
+	auto cb = []() -> bool { return true; };
+	callbacks::Connection<bool>::Cleanup empty;
+	callbacks::Connection<bool>::ConnectedPair conn;
+
+	EXPECT_THROW(conn = callbacks::Connection<bool>::establish(cb, empty),
+	             callbacks::EmptyFunctionObject);
+
+	auto& [handle, callable] = conn;
+
+	// Ordering is important here. If handle.reset() tries blindly to call the
+	// cleanup callback, the exception could be thrown before the connection
+	// is broken. When that happens, the destructor will try to reset again.
+	// By resetting the callable second, there is no need to try the cleanup
+	// funciton again, so the destructor won't throw.
+	EXPECT_NO_THROW(handle.reset());
+	EXPECT_NO_THROW(callable.reset());
+}
+
+// Tests both invalid cleanup and invalid callback function objects
+TEST_F(CallbackTest, EstablishWithNonCallableCallbackAndCleanup) {
+	using namespace uprotocol::utils;
+
+	callbacks::Connection<bool>::Cleanup empty;
+	callbacks::Connection<bool>::ConnectedPair conn;
+
+	EXPECT_THROW(conn = callbacks::Connection<bool>::establish({}, empty),
+	             callbacks::EmptyFunctionObject);
+
+	auto& [handle, callable] = conn;
+
+	// Ordering is important here. If handle.reset() tries blindly to call the
+	// cleanup callback, the exception could be thrown before the connection
+	// is broken. When that happens, the destructor will try to reset again.
+	// By resetting the callable second, there is no need to try the cleanup
+	// funciton again, so the destructor won't throw.
+	EXPECT_NO_THROW(handle.reset());
+	EXPECT_NO_THROW(callable.reset());
+}
+
 }  // namespace