Adding exception type for empty callback functions

include/up-cpp/utils/CallbackConnection.h

@@ -234,6 +234,18 @@ struct BadConnection : public std::runtime_error {
 	    : 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 invokation 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...>;
@@ -254,11 +266,21 @@ struct [[nodiscard]] CalleeHandle {
 			    "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

test/coverage/communication/NotificationSinkTest.cpp

@@ -216,14 +216,23 @@ TEST_F(NotificationSinkTest, NullCallback) {
 	    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

@@ -186,14 +186,21 @@ TEST_F(SubscriberTest, SubscribeNullCallback) {
 	    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

@@ -698,4 +698,73 @@ TEST_F(CallbackTest, CalleeHandleCanDefaultConstruct) {
 	});
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// 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