diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 05a0d8f81..a8855b4f9 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,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: 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. diff --git a/include/up-cpp/utils/CallbackConnection.h b/include/up-cpp/utils/CallbackConnection.h index 5d2455241..417f342aa 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 invocation 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; @@ -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 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 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