From 86dfd69802e673d00aed0062f41eddea4670b571 Mon Sep 17 00:00:00 2001 From: Diana Pinto Date: Fri, 4 Oct 2024 16:20:44 +0100 Subject: [PATCH] capicxx-someip-runtime 3.2.4 - Unlock event processing - Fix build on Android - Call registerAvailabilityHandler before requestService - Fix deadlock issue when unregistering services - Added github workflow to build in Ubuntu - Fix: install boost - Build capicxx-someip-runtime on Windows - Fix Copyright & github link in README - Fix MethodStubDispatcher template - (dev) Warn about multiple subscriptions - Improve the CommonAPI-SomeIP Mainloop logs - Fix to check for the entry containing a valid Message obj to avoid nullptr - Fix Windows build --- Android.bp | 4 +-- Android.mk | 3 +- CHANGES | 18 +++++++++++ CMakeLists.txt | 14 ++++++-- include/CommonAPI/SomeIP/Event.hpp | 32 +++++++++++++------ include/CommonAPI/SomeIP/OutputStream.hpp | 2 +- .../CommonAPI/SomeIP/StubAdapterHelper.hpp | 10 +++--- src/CommonAPI/SomeIP/Connection.cpp | 3 +- src/CommonAPI/SomeIP/Factory.cpp | 21 +++++++++--- ...InstanceAvailabilityStatusChangedEvent.cpp | 18 +++++------ src/CommonAPI/SomeIP/Proxy.cpp | 5 +-- src/CommonAPI/SomeIP/Watch.cpp | 26 ++++++++++++--- 12 files changed, 114 insertions(+), 42 deletions(-) diff --git a/Android.bp b/Android.bp index f2be7e9..ec503df 100644 --- a/Android.bp +++ b/Android.bp @@ -6,13 +6,13 @@ cc_library_shared { "-D_GLIBCXX_USE_NANOSLEEP", "-DBOOST_LOG_DYN_LINK", "-pthread", - "-Wno-unused-private-field" + "-Wno-unused-private-field", + "-Wno-error=deprecated-declarations" ], local_include_dirs: [ "include", - "internal", ], shared_libs: [ diff --git a/Android.mk b/Android.mk index 49fd2be..6cdb2c3 100644 --- a/Android.mk +++ b/Android.mk @@ -34,8 +34,7 @@ LOCAL_SRC_FILES += \ src/CommonAPI/SomeIP/Watch.cpp \ LOCAL_C_INCLUDES := \ - $(LOCAL_PATH)/include \ - $(LOCAL_PATH)/internal + $(LOCAL_PATH)/include LOCAL_SHARED_LIBRARIES := \ libboost_log \ diff --git a/CHANGES b/CHANGES index 14f5b90..d90e40b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,24 @@ Changes ======= +v3.2.4 +- Unlock event processing +- Fix build on Android +- Call registerAvailabilityHandler before requestService +- Fix deadlock issue when unregistering services +- Added github workflow to build in Ubuntu +- Fix: install boost +- Build capicxx-someip-runtime on Windows + +v3.2.3-r9 +- Fix Copyright & github link in README +- Fix MethodStubDispatcher template +- (dev) Warn about multiple subscriptions +- Improve the CommonAPI-SomeIP Mainloop logs +- Fix to check for the entry containing a valid Message obj to avoid nullptr +- Fix Windows build +- Call registerAvailabilityHandler before requestService + v3.2.3-r8 - Changed level logs from warning to info when address_aliasing is activated - Fix incorrect timings in logs diff --git a/CMakeLists.txt b/CMakeLists.txt index 13cdf49..08eab86 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -11,7 +11,7 @@ set (CMAKE_VERBOSE_MAKEFILE off) set (LIBCOMMONAPI_SOMEIP_MAJOR_VERSION 3) set (LIBCOMMONAPI_SOMEIP_MINOR_VERSION 2) -set (LIBCOMMONAPI_SOMEIP_PATCH_VERSION 3) +set (LIBCOMMONAPI_SOMEIP_PATCH_VERSION 4) message(STATUS "Project name: ${PROJECT_NAME}") @@ -66,7 +66,7 @@ include_directories( ${Boost_INCLUDE_DIR} ) set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /std:c++17 -D_CRT_SECURE_NO_WARNINGS /wd4503") link_directories(${Boost_LIBRARY_DIR}) else() -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Wextra -Wformat -Wformat-security -Wconversion -fexceptions -fstrict-aliasing -fstack-protector -fasynchronous-unwind-tables -fno-omit-frame-pointer -DCOMMONAPI_INTERNAL_COMPILATION -D_GLIBCXX_USE_NANOSLEEP -DBOOST_LOG_DYN_LINK -pthread -fvisibility=hidden") +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -Wall -Wextra -Wformat -Wformat-security -Wconversion -fexceptions -fstrict-aliasing -fstack-protector -fasynchronous-unwind-tables -fno-omit-frame-pointer -DCOMMONAPI_INTERNAL_COMPILATION -D_GLIBCXX_USE_NANOSLEEP -DBOOST_LOG_DYN_LINK -pthread -fvisibility=hidden") endif() SET(MAX_LOG_LEVEL "DEBUG" CACHE STRING "maximum log level") @@ -214,3 +214,13 @@ endif() ############################################################################## # maintainer-clean add_custom_target(maintainer-clean COMMAND rm -rf *) + +############################################################################## +# for debug +if(MAINLOOP_DID_NOT_PROCESS_MESSAGE_MS_THRESHOLD) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DMAINLOOP_DID_NOT_PROCESS_MESSAGE_MS_THRESHOLD") +endif() + +if(MAINLOOP_WATCH_ELEMENTS_SLOW_THRESHOLD) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DMAINLOOP_WATCH_ELEMENTS_SLOW_THRESHOLD") +endif() diff --git a/include/CommonAPI/SomeIP/Event.hpp b/include/CommonAPI/SomeIP/Event.hpp index dd2db98..2e348f1 100644 --- a/include/CommonAPI/SomeIP/Event.hpp +++ b/include/CommonAPI/SomeIP/Event.hpp @@ -70,7 +70,6 @@ class Event: public Events_ { } virtual void onEventMessage(const Message &_message) { - std::lock_guard itsLock(notificationMutex_); if (auto ptr = proxy_.lock()) { event_->handleEventMessage(_message, typename make_sequence::type()); } @@ -85,7 +84,6 @@ class Event: public Events_ { private : std::weak_ptr proxy_; Event* event_; - std::mutex notificationMutex_; }; virtual void onFirstListenerAdded(const Listener&) { @@ -103,6 +101,22 @@ class Event: public Events_ { std::lock_guard itsLock(listeners_mutex_); listeners_.insert(_subscription); } + const std::size_t subscriptionCount = this->getSubscriptionCount(); + if (subscriptionCount > 1) { + std::ostringstream identifiers; + identifiers << std::hex << std::setfill('0') + << std::setw(4) << serviceId_ << '.' + << std::setw(4) << instanceId_ << '.' + << std::setw(4) << eventId_; + + COMMONAPI_WARNING( + "Detected multiple active subscriptions (", + subscriptionCount, + ") to the same event: [", + identifiers.str(), + "]. This is usually unintentional and may lead to poor performance." + ); + } auto major = proxy_.getSomeIpAlias().getMajorVersion(); proxy_.subscribe(serviceId_, instanceId_, eventgroupId_, eventId_, @@ -121,10 +135,10 @@ class Event: public Events_ { template inline void handleEventMessage(const Message &_message, index_sequence) { - + std::tuple itsArguments {arguments_}; InputStream InputStream(_message, isLittleEndian_); if (SerializableArguments::deserialize( - InputStream, std::get(arguments_)...)) { + InputStream, std::get(itsArguments)...)) { if (_message.isInitialValue()) { std::set subscribers; { @@ -133,21 +147,21 @@ class Event: public Events_ { listeners_.clear(); } for(auto const &subscription : subscribers) { - if(!_message.isValidCRC()) { + if (!_message.isValidCRC()) { this->notifySpecificError(subscription, CommonAPI::CallStatus::INVALID_VALUE); } else { - this->notifySpecificListener(subscription, std::get(arguments_)...); + this->notifySpecificListener(subscription, std::get(itsArguments)...); } } } else { - if(!_message.isValidCRC()) { + if (!_message.isValidCRC()) { this->notifyErrorListeners(CommonAPI::CallStatus::INVALID_VALUE); } else { { std::lock_guard itsLock(listeners_mutex_); listeners_.clear(); } - this->notifyListeners(std::get(arguments_)...); + this->notifyListeners(std::get(itsArguments)...); } } } else { @@ -166,9 +180,9 @@ class Event: public Events_ { const event_type_e eventType_; const reliability_type_e reliabilityType_; const bool isLittleEndian_; - std::tuple arguments_; std::mutex listeners_mutex_; std::set listeners_; + const std::tuple arguments_; // the deployments }; } // namespace SomeIP diff --git a/include/CommonAPI/SomeIP/OutputStream.hpp b/include/CommonAPI/SomeIP/OutputStream.hpp index 8e965d6..8458f08 100644 --- a/include/CommonAPI/SomeIP/OutputStream.hpp +++ b/include/CommonAPI/SomeIP/OutputStream.hpp @@ -568,7 +568,7 @@ class OutputStream: public CommonAPI::OutputStream { byte_t raw[sizeof(Type_)]; } value; value.typed = _value; -#ifndef _WIN32 +#if !defined(_WIN32) && !defined(__ANDROID__) if ((__BYTE_ORDER == __LITTLE_ENDIAN) != isLittleEndian_) { #else if (!isLittleEndian_) { diff --git a/include/CommonAPI/SomeIP/StubAdapterHelper.hpp b/include/CommonAPI/SomeIP/StubAdapterHelper.hpp index 8a366de..f701181 100644 --- a/include/CommonAPI/SomeIP/StubAdapterHelper.hpp +++ b/include/CommonAPI/SomeIP/StubAdapterHelper.hpp @@ -338,7 +338,7 @@ class MethodStubDispatcher, DeplIn_> (_stub.get()->*stubFunctor_)( itsClientId, - std::move(std::get(in))... + std::move(std::get(in).getValue())... ); return true; @@ -405,7 +405,7 @@ class MethodWithReplyStubDispatcher< { std::lock_guard lock(mutex_); auto itsMessage = pending_.find(_call); - if (itsMessage != pending_.end()) { + if (itsMessage != pending_.end() && itsMessage->second) { Message itsReply = itsMessage->second.createResponseMessage(); pending_[_call] = itsReply; } else { @@ -504,7 +504,7 @@ class MethodWithReplyStubDispatcher< std::lock_guard lock(mutex_); auto reply = pending_.find(_call); - if (reply != pending_.end()) { + if (reply != pending_.end() && reply->second) { if (sizeof...(DeplOutArgs_) > 0) { OutputStream output(reply->second, isLittleEndian_); if (!SerializableArguments...>::serialize( @@ -600,7 +600,7 @@ class MethodWithReplyStubDispatcher< { std::lock_guard lock(this->mutex_); auto itsMessage = this->pending_.find(_call); - if (itsMessage != this->pending_.end()) { + if (itsMessage != this->pending_.end() && itsMessage->second) { // TODO create error response message Message itsReply = itsMessage->second.createResponseMessage(); this->pending_[_call] = itsReply; @@ -689,7 +689,7 @@ class MethodWithReplyStubDispatcher< std::lock_guard lock(this->mutex_); auto itsReply = this->pending_.find(_call); - if (itsReply != this->pending_.end()) { + if (itsReply != this->pending_.end() && itsReply->second) { if (sizeof...(DeplOutArgs_) > 0) { OutputStream output(itsReply->second, this->isLittleEndian_); if (!SerializableArguments...>::serialize( diff --git a/src/CommonAPI/SomeIP/Connection.cpp b/src/CommonAPI/SomeIP/Connection.cpp index caad05f..af1c486 100644 --- a/src/CommonAPI/SomeIP/Connection.cpp +++ b/src/CommonAPI/SomeIP/Connection.cpp @@ -101,8 +101,9 @@ void Connection::handleProxyReceive(const std::shared_ptr &_me // We must not hold the lock when calling the handlers! for (auto handler : handlers) { - if(auto itsHandler = handler.second.lock()) + if(auto itsHandler = handler.second.lock()) { itsHandler->onEventMessage(Message(_message)); + } } return; diff --git a/src/CommonAPI/SomeIP/Factory.cpp b/src/CommonAPI/SomeIP/Factory.cpp index 2c128d1..c434543 100644 --- a/src/CommonAPI/SomeIP/Factory.cpp +++ b/src/CommonAPI/SomeIP/Factory.cpp @@ -192,6 +192,13 @@ Factory::registerStub( } CommonAPI::Address address(_domain, itsInterface, _instance); + + if (isRegisteredService(address.getAddress())) { + COMMONAPI_ERROR("Registering stub for \"", _domain, ":", _interface, ":", _instance, + "\" failed (Already registered)."); + return false; + } + Address someipAddress; AddressTranslator::get()->translate(address, someipAddress); @@ -238,6 +245,13 @@ Factory::registerStub( } CommonAPI::Address address(_domain, itsInterface, _instance); + + if (isRegisteredService(address.getAddress())) { + COMMONAPI_ERROR("Registering stub for \"", _domain, ":", _interface, ":", _instance, + "\" failed (Already registered)."); + return false; + } + Address someipAddress; if (AddressTranslator::get()->translate(address, someipAddress)) { std::shared_ptr itsConnection = getConnection(_context); @@ -291,13 +305,12 @@ Factory::unregisterStub(const std::string &_domain, COMMONAPI_INFO("Deregistering stub for \"", _domain, ":", _interface, ":", _instance, "\""); - servicesMutex_.lock(); + std::unique_lock itsLock(servicesMutex_); CommonAPI::Address address(_domain, _interface, _instance); const auto &adapterResult = services_.find(address.getAddress()); if (adapterResult == services_.end()) { - servicesMutex_.unlock(); COMMONAPI_WARNING("Deregistering stub for \"", _domain, ":", _interface, ":", _instance, "\" failed (Not registered or already unregistered)."); return false; @@ -311,13 +324,12 @@ Factory::unregisterStub(const std::string &_domain, stubManager->unregisterStubAdapter(adapter); if(!services_.erase(address.getAddress())) { - servicesMutex_.unlock(); COMMONAPI_INFO("Deregistering stub for \"", _domain, ":", _interface, ":", _instance, "\" failed (Removal failed)."); return false; } - servicesMutex_.unlock(); + itsLock.unlock(); decrementConnection(connection); @@ -352,6 +364,7 @@ Factory::createStubAdapter( bool Factory::isRegisteredService(const std::string &_address) { + std::lock_guard itsLock(servicesMutex_); auto serviceIterator = services_.find(_address); if (serviceIterator != services_.end()) { return true; diff --git a/src/CommonAPI/SomeIP/InstanceAvailabilityStatusChangedEvent.cpp b/src/CommonAPI/SomeIP/InstanceAvailabilityStatusChangedEvent.cpp index 7ba99de..d533477 100644 --- a/src/CommonAPI/SomeIP/InstanceAvailabilityStatusChangedEvent.cpp +++ b/src/CommonAPI/SomeIP/InstanceAvailabilityStatusChangedEvent.cpp @@ -19,17 +19,9 @@ InstanceAvailabilityStatusChangedEvent::InstanceAvailabilityStatusChangedEvent( observedInterfaceName_(_interfaceName), observedInterfaceServiceId_ (_serviceId), availabilityHandlerId_(0) { - Address wildcardAddress(observedInterfaceServiceId_, vsomeip::ANY_INSTANCE, vsomeip::ANY_MAJOR, vsomeip::ANY_MINOR); - proxy_.getConnection()->requestService(wildcardAddress); } InstanceAvailabilityStatusChangedEvent::~InstanceAvailabilityStatusChangedEvent() { - Address wildcardAddress(observedInterfaceServiceId_, vsomeip::ANY_INSTANCE, vsomeip::ANY_MAJOR, vsomeip::ANY_MINOR); - - proxy_.getConnection()->unregisterAvailabilityHandler( - wildcardAddress, availabilityHandlerId_); - - proxy_.getConnection()->releaseService(wildcardAddress); } void @@ -98,13 +90,19 @@ InstanceAvailabilityStatusChangedEvent::onFirstListenerAdded( std::weak_ptr itsProxy = proxy_.shared_from_this(); availabilityHandlerId_ = proxy_.getConnection()->registerAvailabilityHandler( wildcardAddress, onServiceInstanceStatus, itsProxy, this); + + proxy_.getConnection()->requestService(wildcardAddress); } void InstanceAvailabilityStatusChangedEvent::onLastListenerRemoved( const Listener &) { - // Destruktor of InstanceAvailabilityStatusChangedEvent will remove the availability handler - // As its needed anyways even if a user never subscribes on this event + Address wildcardAddress(observedInterfaceServiceId_, vsomeip::ANY_INSTANCE, vsomeip::ANY_MAJOR, vsomeip::ANY_MINOR); + + proxy_.getConnection()->unregisterAvailabilityHandler( + wildcardAddress, availabilityHandlerId_); + + proxy_.getConnection()->releaseService(wildcardAddress); } bool diff --git a/src/CommonAPI/SomeIP/Proxy.cpp b/src/CommonAPI/SomeIP/Proxy.cpp index 22e6091..4193a9a 100644 --- a/src/CommonAPI/SomeIP/Proxy.cpp +++ b/src/CommonAPI/SomeIP/Proxy.cpp @@ -289,8 +289,6 @@ bool Proxy::init() { if (!connection) return false; - connection->requestService(alias_); - std::weak_ptr itsProxy = shared_from_this(); availabilityHandlerId_ = connection->registerAvailabilityHandler( alias_, @@ -303,6 +301,9 @@ bool Proxy::init() { std::placeholders::_5), itsProxy, NULL); + + connection->requestService(alias_); + if (connection->isAvailable(alias_)) { std::lock_guard itsLock(availabilityMutex_); availabilityStatus_ = AvailabilityStatus::AVAILABLE; diff --git a/src/CommonAPI/SomeIP/Watch.cpp b/src/CommonAPI/SomeIP/Watch.cpp index af0376b..afcf69e 100644 --- a/src/CommonAPI/SomeIP/Watch.cpp +++ b/src/CommonAPI/SomeIP/Watch.cpp @@ -338,13 +338,15 @@ void Watch::processQueueEntry(std::shared_ptr _queueEntry) { } void Watch::supervise() { + std::string applicationName = ""; { auto itsConnection = connection_.lock(); if (itsConnection) { - COMMONAPI_INFO("Supervising watch[", itsConnection->getName(), "] --> (", + applicationName = itsConnection->getName(); + COMMONAPI_INFO("Supervising watch[", applicationName, "] --> (", max_processing_time_, ", ", max_queue_size_, ")"); } else { - COMMONAPI_WARNING("Supervising watch[] --> (", + COMMONAPI_WARNING("Supervising watch[", applicationName, "] --> (", max_processing_time_, ", ", max_queue_size_, ")"); } } @@ -365,13 +367,28 @@ void Watch::supervise() { itsQueueSize = queue_.size(); } +#ifdef MAINLOOP_WATCH_ELEMENTS_SLOW_THRESHOLD + if (MAINLOOP_WATCH_ELEMENTS_SLOW_THRESHOLD >= itsQueueSize) { + COMMONAPI_WARNING("DEBUG -> Aborting application[", applicationName, "] watch elements threshold defined."); + abort(); + } +#endif + +#ifdef MAINLOOP_DID_NOT_PROCESS_MESSAGE_MS_THRESHOLD + if (MAINLOOP_DID_NOT_PROCESS_MESSAGE_MS_THRESHOLD >= itsDuration.count()) { + COMMONAPI_WARNING("DEBUG -> Aborting application[", applicationName, "] ms threshold defined."); + abort(); + } +#endif + if (0 < max_processing_time_ && itsDuration.count() > max_processing_time_) { std::stringstream itsMessage; itsMessage << "Mainloop did not process a message for " << itsDuration.count() << "ms (#elements=" - << itsQueueSize << ")"; + << itsQueueSize << ")" + << " Application[" << applicationName << "]"; COMMONAPI_WARNING(itsMessage.str()); } else if (0 < max_queue_size_ @@ -380,7 +397,8 @@ void Watch::supervise() { std::stringstream itsMessage; itsMessage << "Mainloop watch contains " << itsQueueSize << " elements." - << " Maybe processing is too slow."; + << " Maybe processing is too slow" + << " Application[" << applicationName << "]"; COMMONAPI_WARNING(itsMessage.str()); }