Skip to content

Commit 613789a

Browse files
emiliocuestafMiguelCompany
emiliocuestaf
and
MiguelCompany
authored
Fix handling of InstanceHandle use cases (#6194)
* First attempt to fix the issue Signed-off-by: Emilio Cuesta <[email protected]> * Undoing if removal Signed-off-by: Emilio Cuesta <[email protected]> * Almost done with the RTPS fix Signed-off-by: Emilio Cuesta <[email protected]> * Uncrustify Signed-off-by: Emilio Cuesta <[email protected]> * Update comment Signed-off-by: Emilio Cuesta <[email protected]> * DDS participants should discard now Signed-off-by: Emilio Cuesta <[email protected]> * Adding errors and logs for DDS case Signed-off-by: Emilio Cuesta <[email protected]> * Moving error log Signed-off-by: Emilio Cuesta <[email protected]> * uncrustify Signed-off-by: Emilio Cuesta <[email protected]> * Minor changes and moving logs Signed-off-by: Emilio Cuesta <[email protected]> * Adding some tests and uncrustify Signed-off-by: Emilio Cuesta <[email protected]> * Uncrustify (again) Signed-off-by: Emilio Cuesta <[email protected]> * Adding tests for DDS write Signed-off-by: Emilio Cuesta <[email protected]> * Adding DDS and RTPS writer tests Signed-off-by: Emilio Cuesta <[email protected]> * DISCARDED_BY_UNKNOWN_INSTANCE test ready Signed-off-by: Emilio Cuesta <[email protected]> * Blackbox test to check serialization is working now Signed-off-by: Emilio Cuesta <[email protected]> * Uncrustify Signed-off-by: Emilio Cuesta <[email protected]> * Fix miscalculation in inlineqos size Signed-off-by: Emilio Cuesta <[email protected]> * Apply suggestions from code review Co-authored-by: Miguel Company <[email protected]> Signed-off-by: Emilio Cuesta Fernandez <[email protected]> * Fix tests in Release mode Signed-off-by: Emilio Cuesta <[email protected]> * Attempt to fix mac compilation Signed-off-by: Emilio Cuesta <[email protected]> * Making errors to trigger in more precise situations Signed-off-by: Emilio Cuesta <[email protected]> * Apply suggestions from code review Co-authored-by: Miguel Company <[email protected]> Signed-off-by: Emilio Cuesta Fernandez <[email protected]> * Moving RTPS test to RTPSBlackboxTestsKeys.cpp Signed-off-by: Emilio Cuesta <[email protected]> * Add tests with no handle but with payload and cleanup Signed-off-by: Emilio Cuesta <[email protected]> * Fix broken write functionality in non keyed topics Signed-off-by: Emilio Cuesta <[email protected]> * Applying last batch of reviews Signed-off-by: Emilio Cuesta <[email protected]> * Avoid ABI break by not returning the new rejection reason Signed-off-by: Emilio Cuesta <[email protected]> --------- Signed-off-by: Emilio Cuesta <[email protected]> Signed-off-by: Emilio Cuesta Fernandez <[email protected]> Co-authored-by: Miguel Company <[email protected]>
1 parent 8b08b32 commit 613789a

File tree

16 files changed

+913
-26
lines changed

16 files changed

+913
-26
lines changed

‎include/fastdds/dds/core/status/SampleRejectedStatus.hpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ enum SampleRejectedStatusKind
3737
//! Exceeds the max_samples limit
3838
REJECTED_BY_SAMPLES_LIMIT,
3939
//! Exceeds the max_samples_per_instance limit
40-
REJECTED_BY_SAMPLES_PER_INSTANCE_LIMIT
40+
REJECTED_BY_SAMPLES_PER_INSTANCE_LIMIT,
41+
//! Instance could not be determined
42+
REJECTED_BY_UNKNOWN_INSTANCE
4143
};
4244

4345
//! @brief A struct storing the sample rejected status

‎src/cpp/fastdds/core/policy/ParameterSerializer.hpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ class ParameterSerializer<Parameter_t>
122122
rtps::CDRMessage_t* cdr_message,
123123
const rtps::InstanceHandle_t& iHandle)
124124
{
125+
// Size of parameter key is 20 bytes: 4 for PID (2) and length (2), 16 for the key hash
125126
if (cdr_message->pos + 20 >= cdr_message->max_size)
126127
{
127128
return false;

‎src/cpp/fastdds/publisher/DataWriterImpl.cpp‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -679,13 +679,16 @@ ReturnCode_t DataWriterImpl::check_write_preconditions(
679679
#if HAVE_SECURITY
680680
is_key_protected = writer_->getAttributes().security_attributes().is_key_protected;
681681
#endif // if HAVE_SECURITY
682-
type_.get()->compute_key(data, instance_handle, is_key_protected);
682+
if (!type_->compute_key(data, instance_handle, is_key_protected) || !instance_handle.isDefined())
683+
{
684+
EPROSIMA_LOG_ERROR(DATA_WRITER, "Could not compute key for data");
685+
return RETCODE_PRECONDITION_NOT_MET;
686+
}
683687
}
684688

685-
// Check if the Handle is different from the special value HANDLE_NIL and
686-
// does not correspond with the instance referred by the data
687-
if (handle.isDefined() && handle != instance_handle)
689+
if (handle.isDefined() && instance_handle != handle)
688690
{
691+
EPROSIMA_LOG_ERROR(DATA_WRITER, "Handle differs from data's key.");
689692
return RETCODE_PRECONDITION_NOT_MET;
690693
}
691694

@@ -760,21 +763,25 @@ ReturnCode_t DataWriterImpl::check_instance_preconditions(
760763

761764
instance_handle = handle;
762765

763-
#if defined(NDEBUG)
766+
#if defined(NDEBUG) // In Release build, compute key only if necessary
764767
if (!instance_handle.isDefined())
765768
#endif // if !defined(NDEBUG)
766769
{
767770
bool is_key_protected = false;
768771
#if HAVE_SECURITY
769772
is_key_protected = writer_->getAttributes().security_attributes().is_key_protected;
770773
#endif // if HAVE_SECURITY
771-
type_->compute_key(data, instance_handle, is_key_protected);
774+
if (!type_->compute_key(data, instance_handle, is_key_protected) || !instance_handle.isDefined())
775+
{
776+
EPROSIMA_LOG_ERROR(DATA_WRITER, "Could not compute key for data");
777+
return RETCODE_PRECONDITION_NOT_MET;
778+
}
772779
}
773780

774-
#if !defined(NDEBUG)
781+
#if !defined(NDEBUG) // In Debug build, always check that provided handle matches data's key
775782
if (handle.isDefined() && instance_handle != handle)
776783
{
777-
EPROSIMA_LOG_ERROR(DATA_WRITER, "handle differs from data's key.");
784+
EPROSIMA_LOG_ERROR(DATA_WRITER, "Handle differs from data's key.");
778785
return RETCODE_PRECONDITION_NOT_MET;
779786
}
780787
#endif // if !defined(NDEBUG)
@@ -1115,14 +1122,20 @@ ReturnCode_t DataWriterImpl::create_new_change_with_params(
11151122
return ret_code;
11161123
}
11171124

1125+
// As this entry point does not receive an InstanceHandle_t, it has to be computed if the topic
1126+
// requires it
11181127
InstanceHandle_t handle;
11191128
if (type_->is_compute_key_provided)
11201129
{
11211130
bool is_key_protected = false;
11221131
#if HAVE_SECURITY
11231132
is_key_protected = writer_->getAttributes().security_attributes().is_key_protected;
11241133
#endif // if HAVE_SECURITY
1125-
type_->compute_key(data, handle, is_key_protected);
1134+
if (!type_->compute_key(data, handle, is_key_protected) || !handle.isDefined())
1135+
{
1136+
EPROSIMA_LOG_ERROR(DATA_WRITER, "Could not compute key for data");
1137+
return RETCODE_PRECONDITION_NOT_MET;
1138+
}
11261139
}
11271140

11281141
return perform_create_new_change(changeKind, data, wparams, handle);

‎src/cpp/fastdds/subscriber/DataReaderImpl.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2040,12 +2040,11 @@ InstanceHandle_t DataReaderImpl::lookup_instance(
20402040
const void* instance) const
20412041
{
20422042
InstanceHandle_t handle = HANDLE_NIL;
2043-
20442043
if (instance && type_->is_compute_key_provided)
20452044
{
20462045
if (type_->compute_key(const_cast<void*>(instance), handle, false))
20472046
{
2048-
if (!history_.is_instance_present(handle))
2047+
if (!history_.is_instance_present(handle) || !handle.isDefined())
20492048
{
20502049
handle = HANDLE_NIL;
20512050
}

‎src/cpp/fastdds/subscriber/history/DataReaderHistory.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ bool DataReaderHistory::received_change_keep_all(
221221
return add_to_reader_history_if_not_full(a_change, rejection_reason);
222222
}
223223

224+
FASTDDS_TODO_BEFORE(3, 5, "Change REJECTED_BY_INSTANCES_LIMIT for REJECTED_BY_UNKNOWN_INSTANCE");
224225
rejection_reason = REJECTED_BY_INSTANCES_LIMIT;
225226
return false;
226227
}
@@ -263,6 +264,7 @@ bool DataReaderHistory::received_change_keep_last(
263264
return add_to_reader_history_if_not_full(a_change, rejection_reason);
264265
}
265266

267+
FASTDDS_TODO_BEFORE(3, 5, "Change REJECTED_BY_INSTANCES_LIMIT for REJECTED_BY_UNKNOWN_INSTANCE");
266268
rejection_reason = REJECTED_BY_INSTANCES_LIMIT;
267269
return false;
268270
}

‎src/cpp/rtps/builtin/data/ReaderProxyData.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ uint32_t ReaderProxyData::get_serialized_size(
240240
ret_val += dds::ParameterSerializer<Parameter_t>::cdr_serialized_size(type_name);
241241

242242
// PID_KEY_HASH
243+
// ReaderProxyData always contains the reader's GUID as a key hash
243244
ret_val += 4 + 16;
244245

245246
// PID_PROTOCOL_VERSION

‎src/cpp/rtps/builtin/data/WriterProxyData.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,7 @@ uint32_t WriterProxyData::get_serialized_size(
229229
ret_val += dds::ParameterSerializer<Parameter_t>::cdr_serialized_size(type_name);
230230

231231
// PID_KEY_HASH
232+
// WriterProxyData always contains the writer's GUID as a key hash
232233
ret_val += 4 + 16;
233234

234235
// PID_TYPE_MAX_SIZE_SERIALIZED

‎src/cpp/rtps/history/WriterHistory.cpp‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,6 @@ static CacheChange_t* initialize_change(
6161
RTPSWriter* writer)
6262
{
6363
reserved_change->kind = change_kind;
64-
if ((WITH_KEY == writer->getAttributes().topicKind) && !handle.isDefined())
65-
{
66-
EPROSIMA_LOG_WARNING(RTPS_WRITER, "Changes in KEYED Writers need a valid instanceHandle");
67-
}
6864
reserved_change->instanceHandle = handle;
6965
reserved_change->writerGUID = writer->getGuid();
7066
reserved_change->writer_info.previous = nullptr;
@@ -155,6 +151,13 @@ bool WriterHistory::prepare_and_add_change(
155151
"' bytes and cannot be resized.");
156152
return false;
157153
}
154+
if (TopicKind_t::WITH_KEY == mp_writer->getAttributes().topicKind && !a_change->instanceHandle.isDefined() &&
155+
a_change->kind != ALIVE && a_change->serializedPayload.length == 0)
156+
{
157+
EPROSIMA_LOG_ERROR(RTPS_WRITER_HISTORY,
158+
"Changes of type not equal to ALIVE in KEYED Writers need a valid instanceHandle or the payload to be transmitted");
159+
return false;
160+
}
158161

159162
if (m_isHistoryFull)
160163
{
@@ -483,10 +486,17 @@ void WriterHistory::set_fragments(
483486
{
484487
inline_qos_size += 4u;
485488
}
486-
if (ChangeKind_t::ALIVE != change->kind && TopicKind_t::WITH_KEY == mp_writer->getAttributes().topicKind)
489+
if (change->instanceHandle.isDefined() && TopicKind_t::WITH_KEY == mp_writer->getAttributes().topicKind)
487490
{
491+
// KEY_HASH inlineQoS could be added even if the change is ALIVE. It could always be sent.
492+
// The only restriction is that it MUST be present if the change is not ALIVE (DISPOSE or UNREGISTER).
493+
// It is sent it always as long as the instanceHandle is defined.
488494
inline_qos_size += fastdds::dds::ParameterSerializer<Parameter_t>::PARAMETER_KEY_SIZE;
489-
inline_qos_size += fastdds::dds::ParameterSerializer<Parameter_t>::PARAMETER_STATUS_SIZE;
495+
if (change->kind != ALIVE)
496+
{
497+
// If the change is not ALIVE, STATUS inlineQoS will also be added.
498+
inline_qos_size += fastdds::dds::ParameterSerializer<Parameter_t>::PARAMETER_STATUS_SIZE;
499+
}
490500
}
491501

492502
// If inlineqos for related_sample_identity is required, then remove its size from the final fragment size.

‎src/cpp/rtps/messages/RTPSMessageGroup.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,7 +631,8 @@ bool RTPSMessageGroup::add_data(
631631
if (!RTPSMessageCreator::addSubmessageData(submessage_msg_, &change_to_add, endpoint_->getAttributes().topicKind,
632632
readerId, expectsInlineQos, inline_qos, is_big_submessage, copy_data, pending_buffer_, pending_padding_))
633633
{
634-
EPROSIMA_LOG_ERROR(RTPS_WRITER, "Cannot add DATA submsg to the CDRMessage. Buffer too small");
634+
EPROSIMA_LOG_ERROR(RTPS_WRITER,
635+
"Cannot add DATA submsg to the CDRMessage. Either buffer is too small or handle was required and not set");
635636
change_to_add.serializedPayload.data = nullptr;
636637
return false;
637638
}
@@ -750,7 +751,8 @@ bool RTPSMessageGroup::add_data_frag(
750751
change_to_add.serializedPayload, endpoint_->getAttributes().topicKind, readerId,
751752
expectsInlineQos, inline_qos, copy_data, pending_buffer_, pending_padding_))
752753
{
753-
EPROSIMA_LOG_ERROR(RTPS_WRITER, "Cannot add DATA_FRAG submsg to the CDRMessage. Buffer too small");
754+
EPROSIMA_LOG_ERROR(RTPS_WRITER,
755+
"Cannot add DATA_FRAG submsg to the CDRMessage. Either buffer is too small or handle was required and not set");
754756
change_to_add.serializedPayload.data = nullptr;
755757
return false;
756758
}

‎src/cpp/rtps/messages/submessages/DataMsg.hpp‎

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -147,16 +147,29 @@ struct DataMsgUtils
147147
msg, change->write_params.original_writer_info());
148148
}
149149

150-
if (WITH_KEY == topicKind && (!change->writerGUID.is_builtin() || expectsInlineQos || ALIVE != change->kind))
150+
if (WITH_KEY == topicKind && change->instanceHandle.isDefined() &&
151+
(!change->writerGUID.is_builtin() || expectsInlineQos || ALIVE != change->kind))
151152
{
153+
/**
154+
* If instanceHandle is not defined, this means the key hash is not populated. It makes no sense
155+
* to serialize a parameter with an undefined or empty value because it could be interpreted as
156+
* a 'valid' key hash on the reader side
157+
**/
152158
fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_key(msg,
153159
change->instanceHandle);
154160

161+
/** Changes like UNREGISTER or DISPOSE must include the key, they can't be sent without key */
155162
if (ALIVE != change->kind)
156163
{
157164
fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_status(msg, status);
158165
}
159166
}
167+
else if (ALIVE != change->kind && change->serializedPayload.length > 0)
168+
{
169+
// This case is added in order to support DISPOSE or UNREGISTER changes when the data
170+
// is passed in the payload. Thus the KEY_HASH inline QoS is not compulsory for us
171+
fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_status(msg, status);
172+
}
160173

161174
if (inlineQos != nullptr)
162175
{
@@ -257,6 +270,22 @@ bool RTPSMessageCreator::addSubmessageData(
257270
//Add INLINE QOS AND SERIALIZED PAYLOAD DEPENDING ON FLAGS:
258271
if (inlineQosFlag) //inlineQoS
259272
{
273+
if (WITH_KEY == topicKind &&
274+
change->instanceHandle.isDefined() == false &&
275+
change->kind != ALIVE && change->serializedPayload.length == 0)
276+
{
277+
// Instance handle is required but not defined
278+
EPROSIMA_LOG_ERROR(RTPS_WRITER,
279+
"DISPOSE or UNREGISTER Changes in KEYED Writers need a valid instanceHandle or the payload to be transmitted. Message won't be serialized");
280+
return false;
281+
}
282+
if (WITH_KEY == topicKind &&
283+
change->instanceHandle.isDefined() == false)
284+
{
285+
// Instance handle should be defined but is not compulsory, this is just a warning
286+
EPROSIMA_LOG_WARNING(RTPS_WRITER,
287+
"Change does not have a valid instanceHandle. KEY_HASH will not be serialized.");
288+
}
260289
DataMsgUtils::serialize_inline_qos(msg, change, topicKind, expectsInlineQos, inlineQos, status);
261290
}
262291

@@ -292,11 +321,16 @@ bool RTPSMessageCreator::addSubmessageData(
292321
}
293322

294323
added_no_error &= CDRMessage::addUInt16(msg, 0); //ENCAPSULATION OPTIONS
295-
added_no_error &=
296-
fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_key(msg,
297-
change->instanceHandle);
298-
added_no_error &=
299-
fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_status(msg,
324+
325+
if (change->instanceHandle.isDefined())
326+
{
327+
// Even if requested in the serializedPayload, it makes no sense an undefined key hash
328+
added_no_error &=
329+
fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_key(msg,
330+
change->instanceHandle);
331+
}
332+
333+
added_no_error &= fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_status(msg,
300334
status);
301335
added_no_error &= fastdds::dds::ParameterSerializer<fastdds::dds::Parameter_t>::add_parameter_sentinel(msg);
302336
}
@@ -442,6 +476,22 @@ bool RTPSMessageCreator::addSubmessageDataFrag(
442476
//Add INLINE QOS AND SERIALIZED PAYLOAD DEPENDING ON FLAGS:
443477
if (inlineQosFlag) //inlineQoS
444478
{
479+
if (WITH_KEY == topicKind &&
480+
change->instanceHandle.isDefined() == false &&
481+
change->kind != ALIVE && change->serializedPayload.length == 0)
482+
{
483+
// Instance handle is required but not defined
484+
EPROSIMA_LOG_ERROR(RTPS_WRITER,
485+
"DISPOSE or UNREGISTER Changes in KEYED Writers need a valid instanceHandle or the payload to be transmitted. Message won't be serialized");
486+
return false;
487+
}
488+
if (WITH_KEY == topicKind &&
489+
change->instanceHandle.isDefined() == false)
490+
{
491+
// Instance handle should be defined but is not compulsory, this is just a warning
492+
EPROSIMA_LOG_WARNING(RTPS_WRITER,
493+
"Change does not have a valid instanceHandle. KEY_HASH will not be serialized.");
494+
}
445495
DataMsgUtils::serialize_inline_qos(msg, change, topicKind, expectsInlineQos, inlineQos, status);
446496
}
447497

0 commit comments

Comments
 (0)