Repeated matched event notification

[Barry-Xu-2018]

Is there an already existing issue for this?

• I have searched the existing issues

Expected behavior

Only one matched event notification

Current behavior

Double matched event notification

Steps to reproduce

Environment is ros2 rolling.
Demos: https://github.com/Barry-Xu-2018/demos/tree/topic-add-matched-event-demo

• Run demo

$ ros2 run demo_nodes_cpp matched_event_detect

• Run topic echo on another console

ros2 topic echo /pub_matched_event_detect

Expected output for demo

[INFO] [1679575430.352691543] [matched_event_detection_node]: 1 1 1 1
[INFO] [1679575430.353061496] [matched_event_detection_node]: First subscription is connected.

Actual output

[INFO] [1679576131.704595801] [matched_event_detection_node]: 1 1 1 1
[INFO] [1679576131.704873287] [matched_event_detection_node]: First subscription is connected.
[INFO] [1679576131.705199244] [matched_event_detection_node]: 1 0 1 0
[INFO] [1679576131.705271252] [matched_event_detection_node]: Current number of connected subscription is 1

Fast DDS version/commit

master (commit: 0a43497)

Platform/Architecture

Ubuntu Focal 20.04 amd64, Other. Please specify in Additional context section.

Transport layer

Default configuration, UDPv4 & SHM

Additional context

After checking code, I think the below codes should be changed

Fast-DDS/src/cpp/fastdds/publisher/DataWriterImpl.cpp

Lines 1343 to 1349 in 0a43497

	if (listener != nullptr)
	{
	listener->on_publication_matched(user_datawriter_, publication_matched_status_);
	publication_matched_status_.current_count_change = 0;
	publication_matched_status_.total_count_change = 0;
	}
	user_datawriter_->get_statuscondition().get_impl()->set_status(notify_status, true);

Not like other method process, it doesn't call get_publication_matched_status().
In get_publication_matched_status(), it will call below code to clear status.

Fast-DDS/src/cpp/fastdds/publisher/DataWriterImpl.cpp

Line 1368 in 0a43497

user_datawriter_->get_statuscondition().get_impl()->set_status(StatusMask::publication_matched(), false);

About 2 notification:
First notification is here
listener->on_publication_matched() will call code in rmw_fastrtps
https://github.com/ros2/rmw_fastrtps/blob/f13df4c79ed867488e607251a25c784643724a3e/rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp#L368

Second notification is here

Fast-DDS/src/cpp/fastdds/publisher/DataWriterImpl.cpp

Line 1349 in 0a43497

user_datawriter_->get_statuscondition().get_impl()->set_status(notify_status, true);

Finally, they will be processed at
https://github.com/ros2/rmw_fastrtps/blob/f13df4c79ed867488e607251a25c784643724a3e/rmw_fastrtps_shared_cpp/src/rmw_wait.cpp#L192-L206

I am not sure where this problem belongs to fastdds or rmw_fastrtp.

I can modify code as the below to solve this problem. But I don't confirm whether above my understanding is correct.

Barry-Xu-2018@baa20a2

XML configuration file

No response

Relevant log output

No response

Network traffic capture

No response

[MiguelCompany]

@Barry-Xu-2018 Thanks for the report.

I think the changes on Barry-Xu-2018@baa20a2 make sense.

Would you mind opening a PR with them?

[Barry-Xu-2018]

@MiguelCompany

Would you mind opening a PR with them?

Okay. I will make a PR.

[Barry-Xu-2018]

I created a PR #3396 to fix it.
Please help review @MiguelCompany

[MiguelCompany]

@Barry-Xu-2018 After a colleague discovered a regression created by #3396, and we fixed it with #3418, I am revisiting this one.

The behaviour you observed is the one mandated by the DDS standard:

2.2.4.6 Combination
Those two mechanisms may be combined in the application (e.g., using wait-sets and conditions to access the data and 
listeners to be warned asynchronously of erroneous communication statuses).

It is likely that the application will choose one or the other mechanism for each particular communication status (not both). 
However, if both mechanisms are enabled, then the listener mechanism is used first and then the WaitSet objects are signalled

So when using both a listener and a WaitSet for the same communication status, both should be triggered.

[Barry-Xu-2018]

@MiguelCompany Thanks for sharing information.

Based on #3418, I do test on my environment. Repeat issue doesn't occur.
I think calling get_publication_matched_status() and get_subscription_matched_status() also help to clear status to avoid repeated notification.

Fast-DDS/src/cpp/fastdds/publisher/DataWriterImpl.cpp

Lines 1201 to 1202 in 7123d35

	if (ReturnCode_t::RETCODE_OK == data_writer_->get_publication_matched_status(callback_status))
	{

Fast-DDS/src/cpp/fastdds/subscriber/DataReaderImpl.cpp

Line 922 in 7123d35

if (ReturnCode_t::RETCODE_OK == data_reader_->get_subscription_matched_status(callback_status))

[Barry-Xu-2018]

@MiguelCompany

Sorry. I find I used old test codes to do test. So I didn't find a problem.
Next, I will consider how to resolve it. Maybe change in rmw_fastrtps.

[MiguelCompany]

@Barry-Xu-2018 I guess you could ignore the event whenever current_count_change and total_count_change are both 0

[Barry-Xu-2018]

@MiguelCompany

I guess you could ignore the event whenever current_count_change and total_count_change are both 0

Thank you. In callback function, we can ignore it base on your suggestion.

Now, I am considering ignore repeated event.
I think the problem is here (rmw_fastrtps)

https://github.com/ros2/rmw_fastrtps/blob/901339f274fc07fad757fb32bd16f00815217302/rmw_fastrtps_shared_cpp/src/rmw_wait.cpp#L192-L204

       if (guard_condition.get_trigger_value()) {
          active = true;
          guard_condition.set_trigger_value(false);
        } else {
          active = fase;    // I think we should not do notify while trigger value is clear. 
        }