From 4bdf0ba1fe757f42bb10bb98ee12bba09887f272 Mon Sep 17 00:00:00 2001 From: Greg Medding Date: Fri, 16 Aug 2024 19:24:43 -0700 Subject: [PATCH 1/7] Adding exception type for empty callback functions This exception will be thrown when an empty std::function is passed to the callback connection module. Adding the type first so that tests can be written. --- include/up-cpp/utils/CallbackConnection.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/up-cpp/utils/CallbackConnection.h b/include/up-cpp/utils/CallbackConnection.h index 5d2455241..836efc78b 100644 --- a/include/up-cpp/utils/CallbackConnection.h +++ b/include/up-cpp/utils/CallbackConnection.h @@ -234,6 +234,18 @@ struct BadConnection : public std::runtime_error { : std::runtime_error(std::forward(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 + EmptyFunctionObject(Args&&... args) + : std::invalid_argument(std::forward(args)...) {} +}; + template struct [[nodiscard]] CalleeHandle { using Conn = Connection; From be5e0c03148e8441f5f6b8c943c3d8808deeae43 Mon Sep 17 00:00:00 2001 From: Greg Medding Date: Fri, 16 Aug 2024 19:30:29 -0700 Subject: [PATCH 2/7] Add callback tests for empty function objects --- .../coverage/utils/CallbackConnectionTest.cpp | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/test/coverage/utils/CallbackConnectionTest.cpp b/test/coverage/utils/CallbackConnectionTest.cpp index a0ae5fb09..fd265b4ac 100644 --- a/test/coverage/utils/CallbackConnectionTest.cpp +++ b/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::ConnectedPair conn; + + EXPECT_THROW(conn = callbacks::Connection::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::Cleanup empty; + callbacks::Connection::ConnectedPair conn; + + EXPECT_THROW(conn = callbacks::Connection::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::Cleanup empty; + callbacks::Connection::ConnectedPair conn; + + EXPECT_THROW(conn = callbacks::Connection::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 From d5dbfcf42aa82bf8d8abb44f1c0c4d1634a7f064 Mon Sep 17 00:00:00 2001 From: Greg Medding Date: Fri, 16 Aug 2024 19:57:39 -0700 Subject: [PATCH 3/7] Fix null callback assertions in L2 tests Some L2 tests were asserting that the std::bad_function_call exception should be thrown when a bad callback is called. We will be changing that behavior so that the exception is thrown when the callback connection is established, requiring updated test cases. --- .../communication/NotificationSinkTest.cpp | 23 +++++++++++++------ .../coverage/communication/SubscriberTest.cpp | 21 +++++++++++------ 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/test/coverage/communication/NotificationSinkTest.cpp b/test/coverage/communication/NotificationSinkTest.cpp index a17da8722..2a545e36b 100644 --- a/test/coverage/communication/NotificationSinkTest.cpp +++ b/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(); - *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 diff --git a/test/coverage/communication/SubscriberTest.cpp b/test/coverage/communication/SubscriberTest.cpp index bb210d236..c5f8414cc 100644 --- a/test/coverage/communication/SubscriberTest.cpp +++ b/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(); - *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 From 2fcb07fa1233461b1022166aeca170e4907e14bf Mon Sep 17 00:00:00 2001 From: Greg Medding Date: Fri, 16 Aug 2024 20:00:06 -0700 Subject: [PATCH 4/7] Add empty function checks in CalleeHandle The CalleeHandle class is the "owner" of the callback function objects associated with a callback connection. As such, it will check the validity of those function objects and throw the EmptyFunctionObject exception if either the callback or cleanup function is empty / invalid. --- include/up-cpp/utils/CallbackConnection.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/include/up-cpp/utils/CallbackConnection.h b/include/up-cpp/utils/CallbackConnection.h index 836efc78b..c5416992f 100644 --- a/include/up-cpp/utils/CallbackConnection.h +++ b/include/up-cpp/utils/CallbackConnection.h @@ -266,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 From 2f81310dc236c96cfd5aa89ec3f0c965948fb965 Mon Sep 17 00:00:00 2001 From: lukas-he Date: Thu, 13 Feb 2025 16:54:02 +0100 Subject: [PATCH 5/7] Update include/up-cpp/utils/CallbackConnection.h Co-authored-by: Pete LeVasseur --- include/up-cpp/utils/CallbackConnection.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/up-cpp/utils/CallbackConnection.h b/include/up-cpp/utils/CallbackConnection.h index c5416992f..417f342aa 100644 --- a/include/up-cpp/utils/CallbackConnection.h +++ b/include/up-cpp/utils/CallbackConnection.h @@ -239,7 +239,7 @@ struct BadConnection : public std::runtime_error { /// 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. +/// the error appear earlier without waiting for invocation to occur. struct EmptyFunctionObject : public std::invalid_argument { template EmptyFunctionObject(Args&&... args) From fa96512f53ca374d15e3eafa86873096c3868a63 Mon Sep 17 00:00:00 2001 From: Lukas Heppel Date: Mon, 17 Feb 2025 11:51:45 +0100 Subject: [PATCH 6/7] specify clang version 17 in ci --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 05a0d8f81..6faf64ac2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -434,7 +434,8 @@ jobs: style: 'file' # read .clang-format for configuration tidy-checks: '' # Read .clang-tidy for configuration database: build/Release/compile_commands.json - + version: 17 + - name: Run linters on tests continue-on-error: true id: test-linter @@ -447,6 +448,7 @@ jobs: style: 'file' # read .clang-format for configuration tidy-checks: '' # Read .clang-tidy for configuration database: build/Release/compile_commands.json + version: 17 - name: Report lint failure if: steps.source-linter.outputs.checks-failed > 0 || steps.test-linter.outputs.checks-failed > 0 From 6afa273bbd1834f0d9f2654c0df77d1e7341b182 Mon Sep 17 00:00:00 2001 From: lukas-he Date: Mon, 3 Mar 2025 20:20:26 +0100 Subject: [PATCH 7/7] Upgrade up spec version and fix CI (#308) * pin Ubuntu version, replace artifact download in lint step by newly creating the compile_commands * update readme to use alpha4 * fix gcovr bug * see: https://github.com/eclipse-uprotocol/up-cpp/actions/runs/13638110265/job/38121680150?pr=308#step:10:660 --- .github/workflows/ci.yml | 73 ++++++++++++++++++---------------- .github/workflows/coverage.yml | 2 +- README.md | 2 +- 3 files changed, 40 insertions(+), 37 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 05a0d8f81..d2c26cfda 100644 --- a/.github/workflows/ci.yml +++ b/.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,17 +380,17 @@ 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 @@ -398,29 +398,30 @@ jobs: 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,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: Run linters on tests continue-on-error: true id: test-linter @@ -447,6 +449,7 @@ 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 +463,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: diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index d83aeec6e..65cf323d6 100644 --- a/.github/workflows/coverage.yml +++ b/.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" diff --git a/README.md b/README.md index 6b42e598e..52439280b 100644 --- a/README.md +++ b/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.