From c55d9aff538bc6a9adbb236763a687bd755fce25 Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Wed, 15 Jan 2020 11:38:32 +0100 Subject: [PATCH 1/3] Ref. 7290 propagate errors on take_next_data SubscriberHistory::deserialize_change must return the result of the deserialization and SubscriberHistory::takeNextData should propagate errors on deserialization and removal from history --- .../fastrtps/subscriber/SubscriberHistory.h | 2 +- src/cpp/subscriber/SubscriberHistory.cpp | 20 +++++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/include/fastrtps/subscriber/SubscriberHistory.h b/include/fastrtps/subscriber/SubscriberHistory.h index b9791da9fc4..dd1f32971c1 100644 --- a/include/fastrtps/subscriber/SubscriberHistory.h +++ b/include/fastrtps/subscriber/SubscriberHistory.h @@ -191,7 +191,7 @@ class SubscriberHistory: public rtps::ReaderHistory rtps::CacheChange_t* a_change, std::vector& instance_changes); - void deserialize_change( + bool deserialize_change( rtps::CacheChange_t* change, uint32_t ownership_strength, void* data, diff --git a/src/cpp/subscriber/SubscriberHistory.cpp b/src/cpp/subscriber/SubscriberHistory.cpp index 784336b6b7f..b0aca980279 100644 --- a/src/cpp/subscriber/SubscriberHistory.cpp +++ b/src/cpp/subscriber/SubscriberHistory.cpp @@ -276,7 +276,7 @@ bool SubscriberHistory::find_key_for_change( return find_key(a_change, &map_it); } -void SubscriberHistory::deserialize_change( +bool SubscriberHistory::deserialize_change( CacheChange_t* change, uint32_t ownership_strength, void* data, @@ -284,7 +284,11 @@ void SubscriberHistory::deserialize_change( { if (change->kind == ALIVE) { - mp_subImpl->getType()->deserialize(&change->serializedPayload, data); + if (!mp_subImpl->getType()->deserialize(&change->serializedPayload, data)) + { + logError(RTPS_HISTORY, "Deserialization of data failed"); + return false; + } } if (info != nullptr) @@ -307,6 +311,8 @@ void SubscriberHistory::deserialize_change( info->iHandle = change->instanceHandle; info->related_sample_identity = change->write_params.sample_identity(); } + + return true; } bool SubscriberHistory::readNextData( @@ -331,8 +337,7 @@ bool SubscriberHistory::readNextData( logInfo(SUBSCRIBER, mp_reader->getGuid().entityId << ": reading " << change->sequenceNumber); uint32_t ownership = wp && mp_subImpl->getAttributes().qos.m_ownership.kind == EXCLUSIVE_OWNERSHIP_QOS ? wp->ownership_strength() : 0; - deserialize_change(change, ownership, data, info); - return true; + return deserialize_change(change, ownership, data, info); } } return false; @@ -362,9 +367,9 @@ bool SubscriberHistory::takeNextData( " from writer: " << change->writerGUID); uint32_t ownership = wp && mp_subImpl->getAttributes().qos.m_ownership.kind == EXCLUSIVE_OWNERSHIP_QOS ? wp->ownership_strength() : 0; - deserialize_change(change, ownership, data, info); - remove_change_sub(change); - return true; + bool deserialized = deserialize_change(change, ownership, data, info); + bool removed = remove_change_sub(change); + return (deserialized && removed); } } @@ -404,7 +409,6 @@ bool SubscriberHistory::find_key( return false; } - bool SubscriberHistory::remove_change_sub( CacheChange_t* change) { From 8c01f532473c7bcb4259c7fe7ce89c7961fe80b4 Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Wed, 22 Jan 2020 14:15:12 +0100 Subject: [PATCH 2/3] Ref. 7290 add log errors on failure --- src/cpp/subscriber/SubscriberHistory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpp/subscriber/SubscriberHistory.cpp b/src/cpp/subscriber/SubscriberHistory.cpp index b0aca980279..b1120d3cbb7 100644 --- a/src/cpp/subscriber/SubscriberHistory.cpp +++ b/src/cpp/subscriber/SubscriberHistory.cpp @@ -437,7 +437,7 @@ bool SubscriberHistory::remove_change_sub( } if (!found) { - logWarning(SUBSCRIBER, "Change not found, something is wrong"); + logError(RTPS_HISTORY, "Change not found on this key, something is wrong"); } } From d0e62a6ef2d1a6c550de3d487cc28fc5252c6472 Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Thu, 23 Jan 2020 08:04:44 +0100 Subject: [PATCH 3/3] Ref. 7290 Change log category to SUBSCRIBER --- src/cpp/subscriber/SubscriberHistory.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/cpp/subscriber/SubscriberHistory.cpp b/src/cpp/subscriber/SubscriberHistory.cpp index b1120d3cbb7..ca7a53dd1ed 100644 --- a/src/cpp/subscriber/SubscriberHistory.cpp +++ b/src/cpp/subscriber/SubscriberHistory.cpp @@ -91,7 +91,7 @@ bool SubscriberHistory::received_change( { if (mp_reader == nullptr || mp_mutex == nullptr) { - logError(RTPS_HISTORY, "You need to create a Reader with this History before using it"); + logError(SUBSCRIBER, "You need to create a Reader with this History before using it"); return false; } @@ -255,7 +255,7 @@ bool SubscriberHistory::find_key_for_change( { if (!a_change->instanceHandle.isDefined() && mp_subImpl->getType() != nullptr) { - logInfo(RTPS_HISTORY, "Getting Key of change with no Key transmitted") + logInfo(SUBSCRIBER, "Getting Key of change with no Key transmitted") mp_subImpl->getType()->deserialize(&a_change->serializedPayload, mp_getKeyObject); bool is_key_protected = false; #if HAVE_SECURITY @@ -268,7 +268,7 @@ bool SubscriberHistory::find_key_for_change( } else if (!a_change->instanceHandle.isDefined()) { - logWarning(RTPS_HISTORY, "NO KEY in topic: " << mp_subImpl->getAttributes().topic.topicName + logWarning(SUBSCRIBER, "NO KEY in topic: " << mp_subImpl->getAttributes().topic.topicName << " and no method to obtain it";); return false; } @@ -286,7 +286,7 @@ bool SubscriberHistory::deserialize_change( { if (!mp_subImpl->getType()->deserialize(&change->serializedPayload, data)) { - logError(RTPS_HISTORY, "Deserialization of data failed"); + logError(SUBSCRIBER, "Deserialization of data failed"); return false; } } @@ -322,7 +322,7 @@ bool SubscriberHistory::readNextData( { if (mp_reader == nullptr || mp_mutex == nullptr) { - logError(RTPS_HISTORY, "You need to create a Reader with this History before using it"); + logError(SUBSCRIBER, "You need to create a Reader with this History before using it"); return false; } @@ -351,7 +351,7 @@ bool SubscriberHistory::takeNextData( { if (mp_reader == nullptr || mp_mutex == nullptr) { - logError(RTPS_HISTORY, "You need to create a Reader with this History before using it"); + logError(SUBSCRIBER, "You need to create a Reader with this History before using it"); return false; } @@ -414,7 +414,7 @@ bool SubscriberHistory::remove_change_sub( { if (mp_reader == nullptr || mp_mutex == nullptr) { - logError(RTPS_HISTORY, "You need to create a Reader with this History before using it"); + logError(SUBSCRIBER, "You need to create a Reader with this History before using it"); return false; } @@ -437,7 +437,7 @@ bool SubscriberHistory::remove_change_sub( } if (!found) { - logError(RTPS_HISTORY, "Change not found on this key, something is wrong"); + logError(SUBSCRIBER, "Change not found on this key, something is wrong"); } } @@ -456,7 +456,7 @@ bool SubscriberHistory::set_next_deadline( { if (mp_reader == nullptr || mp_mutex == nullptr) { - logError(RTPS_HISTORY, "You need to create a Reader with this History before using it"); + logError(SUBSCRIBER, "You need to create a Reader with this History before using it"); return false; } std::lock_guard guard(*mp_mutex); @@ -486,7 +486,7 @@ bool SubscriberHistory::get_next_deadline( { if (mp_reader == nullptr || mp_mutex == nullptr) { - logError(RTPS_HISTORY, "You need to create a Reader with this History before using it"); + logError(SUBSCRIBER, "You need to create a Reader with this History before using it"); return false; } std::lock_guard guard(*mp_mutex);