Bugfix/1.0.2/194 std function checks

.github/workflows/ci.yml

@@ -12,20 +12,20 @@ permissions:
 jobs:
   build:
     name: Build up-cpp and dependencies
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
 
     steps:
+      - name: Fetch up-cpp
+        uses: actions/checkout@v4
+        with:
+          path: up-cpp
+
       - name: Install Conan
         id: conan
         uses: turtlebrowser/get-conan@main
         with:
           version: 2.3.2
 
-      - name: Fetch up-cpp
-        uses: actions/checkout@v4
-        with:
-          path: up-cpp
-
       - name: Install conan CI profile
         shell: bash
         run: |
@@ -42,7 +42,7 @@ jobs:
       - name: Build up-core-api conan package
         shell: bash
         run: |
-          conan create --version 1.6.0-alpha3 up-conan-recipes/up-core-api/release
+          conan create --version 1.6.0-alpha4 up-conan-recipes/up-core-api/release
 
       - name: Build up-cpp with tests
         shell: bash
@@ -78,7 +78,7 @@ jobs:
 
   test:
     name: Run up-cpp tests
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     needs: build
 
     steps:
@@ -105,7 +105,7 @@ jobs:
 # NOTE: Run dynamic analysis in unit tests
   memcheck:
     name: Run Valgrind Memcheck
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     needs: build
 
     steps:
@@ -173,7 +173,7 @@ jobs:
 
   threadcheck:
     name: Run Valgrind ThreadCheck
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     needs: build
 
     steps:
@@ -242,7 +242,7 @@ jobs:
 
   helgrind:
     name: Run Valgrind Helgrind
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     needs: build
 
     steps:
@@ -311,7 +311,7 @@ jobs:
 
   dhat:
     name: Run Valgrind DHAT
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     needs: build
 
     steps:
@@ -380,47 +380,48 @@ jobs:
 
   lint:
     name: Lint C++ sources
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     needs: build
     permissions:
       contents: write
       pull-requests: read
 
     steps:
-      - name: Get build commands
-        uses: actions/download-artifact@v4
+      - name: Fetch up-cpp
+        uses: actions/checkout@v4
         with:
-          name: compile-commands
+          path: up-cpp
 
       - name: Install Conan
         id: conan
         uses: turtlebrowser/get-conan@main
         with:
           version: 2.3.2
 
-      - name: Create default Conan profile
-        run: conan profile detect
-
-      - name: Get conan cache
-        uses: actions/download-artifact@v4
-        with:
-          name: conan-cache
-
-      - name: Restore conan cache from archive
+      - name: Install conan CI profile
         shell: bash
         run: |
-          conan cache restore conan-cache.tgz
+          conan profile detect
+          cp up-cpp/.github/workflows/ci_conan_profile "$(conan profile path default)"
+          conan profile show
 
-      - name: Fetch up-cpp
+      - name: Fetch up-core-api conan recipe
         uses: actions/checkout@v4
         with:
-          path: up-cpp
+          path: up-conan-recipes
+          repository: eclipse-uprotocol/up-conan-recipes
 
-      - name: Get build artifacts
-        uses: actions/download-artifact@v4
-        with:
-          name: build-artifacts
-          path: up-cpp/build/Release
+      - name: Build up-core-api conan package
+        shell: bash
+        run: |
+          conan create --version 1.6.0-alpha4 up-conan-recipes/up-core-api/release
+
+      - name: Build up-cpp with tests
+        shell: bash
+        run: |
+          cd up-cpp
+          conan install --build=missing .
+          cmake --preset conan-release -DCMAKE_EXPORT_COMPILE_COMMANDS=yes
 
       - name: Run linters on source
         continue-on-error: true
@@ -434,7 +435,9 @@ jobs:
           style: 'file' # read .clang-format for configuration
           tidy-checks: '' # Read .clang-tidy for configuration
           database: build/Release/compile_commands.json
+          version: 12
 
+
       - name: Run linters on tests
         continue-on-error: true
         id: test-linter
@@ -447,6 +450,8 @@ jobs:
           style: 'file' # read .clang-format for configuration
           tidy-checks: '' # Read .clang-tidy for configuration
           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
@@ -460,7 +465,7 @@ jobs:
   # job to signal whether all CI checks have passed.
   ci:
     name: CI status checks
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     needs: [build, test, memcheck, threadcheck, helgrind, dhat]
     if: always()
     steps:

.github/workflows/coverage.yml

@@ -65,7 +65,7 @@ jobs:
         run: |
           cd up-cpp/build/Release
           mkdir -p ../Coverage
-          gcovr -r ../../ --html --html-details -o ../Coverage/index.html -e '.*test.*'
+          gcovr -r ../../ --html --html-details -o ../Coverage/index.html -e '.*test.*' --gcov-ignore-parse-errors negative_hits.warn_once_per_file
           cd ..
           echo "Coverage report can be found here: ../Coverage/index.html"
 

README.md

@@ -24,7 +24,7 @@ implementation, such as [up-transport-zenoh-cpp][zenoh-transport-repo].
 Using the recipes found in [up-conan-recipes][conan-recipe-repo], build these
 Conan packages:
 
-1. [up-core-api][spec-repo]: `conan create --version 1.6.0-alpha3 --build=missing up-core-api/release`
+1. [up-core-api][spec-repo]: `conan create --version 1.6.0-alpha4 --build=missing up-core-api/release`
 
 **NOTE:** all `conan` commands in this document use  Conan 2.x syntax. Please
 adjust accordingly when using Conan 1.x.

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 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...>;
@@ -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