diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2c26cfda..a8855b4f9 100644 --- a/.github/workflows/ci.yml +++ b/.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: | 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