[19696][20409] Fix data race on PDP

[Tempate]

Description

This PR intends to solve a race condition on PDP discovery phase, in which a data UP is received and processed while its corresponding previously received data P is still being processed.

As a result, the participant proxy created and registered can be cleared before calling assignRemoteEndpoints, as the PDP mutex is released just before (on purpose to avoid interlocking problems).

This issue has been reported by TSAN in this particular scenario, when the data P is received in the UDP multicast thread and then the data UP in the unicast one. However, this problem should also affect other scenarios such as:

• Same but with SHM transport.
• Same but receiving data P in unicast thread and data UP in multicast one.
• Losing remote participant's liveliness (events thread) while still processing its data P.

The proposed solution is to copy the received participant proxy data before releasing the PDP mutex, and then passing this copy forward to the methods requiring this information.

@Mergifyio backport 2.12.x 2.10.x 2.6.x

Contributor Checklist

• Commit messages follow the project guidelines.
• The code follows the style guidelines of this project.
• Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
• N/A Any new/modified methods have been properly documented using Doxygen.
• Changes are ABI compatible.
• Changes are API compatible.
• N/A New feature has been added to the versions.md file (if applicable).
• N/A New feature has been documented/Current behavior is correctly described in the documentation.
• Applicable backports have been included in the description.

Reviewer Checklist

• The PR has a milestone assigned.
• Check contributor checklist is correct.
• Check CI results: changes do not issue any warning.
• Check CI results: failing tests are unrelated with the changes.

[Tempate]

@richiprosima please test this

[Tempate]

@richiprosima please test this

[MiguelCompany]

1. There is a test that has failed with segmentation fault both in Mac and Windows
   • Mac
   • Windows
   • I think the failing asan job is telling us where the segfault comes from
2. I don't like the solution of copying the whole ParticipantProxyData, it might be better to change the API of the methods receiving it so they only use the necessary information, and just copy that information

[Tempate]

@richiprosima please test this

[Tempate]

@richiprosima please test this

[juanlofer-eprosima]

@richiprosima please test this

[JesusPoderoso]

@juanlofer-eprosima please address linter issue. Is the linux discovery failed test related with the PR?

[Mario-DL]

Good job in this PR as it effectively solves the data race it describes, I also like the simplification on the PDPClient in order to use the is_connected, avoiding to store the entire ParticipantProxyData. I am approving though leaving a reflection.

Trying to give a second thought on @MiguelCompany suggested, I think this work may admit an even fine-grained solution to avoid copying the entire ParcipantProxyData in stack. But it is also true that we are highly tied to the entire pdata within PDP::assign_remote_endpoints() specially when we are notifying the user with the DiscoveredParticipantInfo. The rest of the methods, matched_reader/writer_add() and assign_low_level_endpoints() do not make an intensive use of the ParticipantProxyData and it would be enough to pass them only some of the fields: some locator lists, the guid and the m_availableBuiltinEndpoints in essence.

Finally, I am attaching a possible patch of a different approach instead of stack-copying, which explores the option of synchronizing the PDP::remove_remote_participant() and PDP::assign_remote_endpoints() only in the case that we are trying to delete a pdata that is is currently being processed. The fix is fast and dirty just to make an idea of the approach and I would still need to think if it would solve all the situations that we are solving by making that stack copy.

pdp_patch_another_approach.txt

Approving !

[Mario-DL]

I included the backports in the PR description

[EduPonz]

@Mergifyio backport 2.12.x 2.10.x 2.6.x

[mergify]

backport 2.12.x 2.10.x 2.6.x

✅ Backports have been created

• #4429 [19696][20409] Fix data race on PDP (backport #4220) has been created for branch 2.12.x
• #4430 [19696][20409] Fix data race on PDP (backport #4220) has been created for branch 2.10.x
• #4431 [19696][20409] Fix data race on PDP (backport #4220) has been created for branch 2.6.x but encountered conflicts

[juanlofer-eprosima]

Good job in this PR as it effectively solves the data race it describes, I also like the simplification on the PDPClient in order to use the is_connected, avoiding to store the entire ParticipantProxyData. I am approving though leaving a reflection.

Trying to give a second thought on @MiguelCompany suggested, I think this work may admit an even fine-grained solution to avoid copying the entire ParcipantProxyData in stack. But it is also true that we are highly tied to the entire pdata within PDP::assign_remote_endpoints() specially when we are notifying the user with the DiscoveredParticipantInfo. The rest of the methods, matched_reader/writer_add() and assign_low_level_endpoints() do not make an intensive use of the ParticipantProxyData and it would be enough to pass them only some of the fields: some locator lists, the guid and the m_availableBuiltinEndpoints in essence.

Finally, I am attaching a possible patch of a different approach instead of stack-copying, which explores the option of synchronizing the PDP::remove_remote_participant() and PDP::assign_remote_endpoints() only in the case that we are trying to delete a pdata that is is currently being processed. The fix is fast and dirty just to make an idea of the approach and I would still need to think if it would solve all the situations that we are solving by making that stack copy.

pdp_patch_another_approach.txt

Approving !

I also agree the solution @MiguelCompany suggested was better, but we decided to proceed with the copy approach given the effort required, and also considering that the root of the problem (releasing the PDP mutex before completing an entire discovery operation, which might lead to inconsistent states apart from data races as the one we are dealing with here) would still remain unsolved. We also discussed that a possible solution to tackle the root of the problem would be to derive all discovery actions to a dedicated thread.

Regarding your proposal, I believe it is a bit risky to wait as some lower level resources might be blocked by that thread, leading to potential deadlocks. And two additional comments:

• I believe that (in remove_remote_participant) locking the PDP mutex should be done after calling wait (to avoid intraprocess deadlock).
• I also believe that a PDP instance could process more than one data P before processing a data uP, so I guess the complexity of this solution would need to grow.

[Mario-DL]

Good job in this PR as it effectively solves the data race it describes, I also like the simplification on the PDPClient in order to use the is_connected, avoiding to store the entire ParticipantProxyData. I am approving though leaving a reflection.
Trying to give a second thought on @MiguelCompany suggested, I think this work may admit an even fine-grained solution to avoid copying the entire ParcipantProxyData in stack. But it is also true that we are highly tied to the entire pdata within PDP::assign_remote_endpoints() specially when we are notifying the user with the DiscoveredParticipantInfo. The rest of the methods, matched_reader/writer_add() and assign_low_level_endpoints() do not make an intensive use of the ParticipantProxyData and it would be enough to pass them only some of the fields: some locator lists, the guid and the m_availableBuiltinEndpoints in essence.
Finally, I am attaching a possible patch of a different approach instead of stack-copying, which explores the option of synchronizing the PDP::remove_remote_participant() and PDP::assign_remote_endpoints() only in the case that we are trying to delete a pdata that is is currently being processed. The fix is fast and dirty just to make an idea of the approach and I would still need to think if it would solve all the situations that we are solving by making that stack copy.
pdp_patch_another_approach.txt
Approving !

I also agree the solution @MiguelCompany suggested was better, but we decided to proceed with the copy approach given the effort required, and also considering that the root of the problem (releasing the PDP mutex before completing an entire discovery operation, which might lead to inconsistent states apart from data races as the one we are dealing with here) would still remain unsolved. We also discussed that a possible solution to tackle the root of the problem would be to derive all discovery actions to a dedicated thread.

Regarding your proposal, I believe it is a bit risky to wait as some lower level resources might be blocked by that thread, leading to potential deadlocks. And two additional comments:

• I believe that (in remove_remote_participant) locking the PDP mutex should be done after calling wait (to avoid intraprocess deadlock).
• I also believe that a PDP instance could process more than one data P before processing a data uP, so I guess the complexity of this solution would need to grow.

Yeah, agree that it would be risky as a consequence of relying on a cond var.

A further lock-free approach I can think of would be that the thread executing remove_remote_participant() with the participant proxy that is being currently processed in assign_remote_endpoints , could leave a flag to true in PDPListener meaning that “I am not removing the participant proxy now, as you are using it, but I notify you that when you finish the assign_remote_endpoints() you will have to delete it on behalf of me”
So, in that case, the same discovery thread could call remove_remote_participant() if the flag is up, after executing the assign_remote_endpoints() and then reset it. What do you think ?

[juanlofer-eprosima]

Good job in this PR as it effectively solves the data race it describes, I also like the simplification on the PDPClient in order to use the is_connected, avoiding to store the entire ParticipantProxyData. I am approving though leaving a reflection.
Trying to give a second thought on @MiguelCompany suggested, I think this work may admit an even fine-grained solution to avoid copying the entire ParcipantProxyData in stack. But it is also true that we are highly tied to the entire pdata within PDP::assign_remote_endpoints() specially when we are notifying the user with the DiscoveredParticipantInfo. The rest of the methods, matched_reader/writer_add() and assign_low_level_endpoints() do not make an intensive use of the ParticipantProxyData and it would be enough to pass them only some of the fields: some locator lists, the guid and the m_availableBuiltinEndpoints in essence.
Finally, I am attaching a possible patch of a different approach instead of stack-copying, which explores the option of synchronizing the PDP::remove_remote_participant() and PDP::assign_remote_endpoints() only in the case that we are trying to delete a pdata that is is currently being processed. The fix is fast and dirty just to make an idea of the approach and I would still need to think if it would solve all the situations that we are solving by making that stack copy.
pdp_patch_another_approach.txt
Approving !

I also agree the solution @MiguelCompany suggested was better, but we decided to proceed with the copy approach given the effort required, and also considering that the root of the problem (releasing the PDP mutex before completing an entire discovery operation, which might lead to inconsistent states apart from data races as the one we are dealing with here) would still remain unsolved. We also discussed that a possible solution to tackle the root of the problem would be to derive all discovery actions to a dedicated thread.
Regarding your proposal, I believe it is a bit risky to wait as some lower level resources might be blocked by that thread, leading to potential deadlocks. And two additional comments:

• I believe that (in remove_remote_participant) locking the PDP mutex should be done after calling wait (to avoid intraprocess deadlock).
• I also believe that a PDP instance could process more than one data P before processing a data uP, so I guess the complexity of this solution would need to grow.

Yeah, agree that it would be risky as a consequence of relying on a cond var.

A further lock-free approach I can think of would be that the thread executing remove_remote_participant() with the participant proxy that is being currently processed in assign_remote_endpoints , could leave a flag to true in PDPListener meaning that “I am not removing the participant proxy now, as you are using it, but I notify you that when you finish the assign_remote_endpoints() you will have to delete it on behalf of me” So, in that case, the same discovery thread could call remove_remote_participant() if the flag is up, after executing the assign_remote_endpoints() and then reset it. What do you think ?

That's an interesting approach we could further explore. The only caveat I see at the moment is that remove_remote_participant can also be called from places other than a PDPListener, like for example from the events thread when liveliness is lost.