[19939] TCP deadlock on channel reuse #4099

jepemi · 2023-12-05T06:37:09Z

Description

TCP channel reuse required joining the thread in charge of the previous channel. However, this call to join() involved taking some mutex that prevented the old thread from finishing its tasks, leading to a deadlock.
The disabling of the old channel has been relocated as well as the protection of some other channel resources.

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

Fixes #4026

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.

test/blackbox/common/TCPReqRepHelloWorldReplier.cpp

test/blackbox/common/TCPReqRepHelloWorldReplier.hpp

…k protected Signed-off-by: Jesus Perez <jesusperez@eprosima.com>

Signed-off-by: Jesus Perez <jesusperez@eprosima.com>

jepemi · 2023-12-12T06:36:26Z

@richiprosima please test this

EduPonz · 2023-12-12T09:57:14Z

@richiprosima please test mac

EduPonz · 2023-12-12T14:20:04Z

@richiprosima please test mac

EduPonz · 2023-12-12T15:19:00Z

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

mergify · 2023-12-12T15:19:34Z

backport 2.12.x 2.11.x 2.10.x 2.6.x

✅ Backports have been created

#4127 [19939] TCP deadlock on channel reuse (backport #4099) has been created for branch 2.12.x
#4128 [19939] TCP deadlock on channel reuse (backport #4099) has been created for branch 2.11.x
#4129 [19939] TCP deadlock on channel reuse (backport #4099) has been created for branch 2.10.x
#4130 [19939] TCP deadlock on channel reuse (backport #4099) has been created for branch 2.6.x

* Refs #19939: Channel disabling relocated and OnDataReceived() callback protected Signed-off-by: Jesus Perez <jesusperez@eprosima.com> * Refs #19939: Uncrustify Signed-off-by: Jesus Perez <jesusperez@eprosima.com> * Refs #19939: Apply suggestions Signed-off-by: Jesus Perez <jesusperez@eprosima.com> --------- Signed-off-by: Jesus Perez <jesusperez@eprosima.com> (cherry picked from commit dd4c434)

* Refs #19939: Channel disabling relocated and OnDataReceived() callback protected Signed-off-by: Jesus Perez <jesusperez@eprosima.com> * Refs #19939: Uncrustify Signed-off-by: Jesus Perez <jesusperez@eprosima.com> * Refs #19939: Apply suggestions Signed-off-by: Jesus Perez <jesusperez@eprosima.com> --------- Signed-off-by: Jesus Perez <jesusperez@eprosima.com> (cherry picked from commit dd4c434) Co-authored-by: Jesús Pérez <78275223+jepemi@users.noreply.github.com>

This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Revert "TCP deadlock on channel reuse (#4099)" This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Mark large_data tests as flaky due to TCP Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Revert "TCP deadlock on channel reuse (#4099)" This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Mark large_data tests as flaky due to TCP Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com> (cherry picked from commit 5e87eb3) # Conflicts: # test/blackbox/common/BlackboxTestsTransportCustom.cpp # test/blackbox/xfail_tests.cmake

* Revert "TCP deadlock on channel reuse (#4099)" This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Mark large_data tests as flaky due to TCP Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Revert "TCP deadlock on channel reuse (#4099)" This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Mark large_data tests as flaky due to TCP Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com> (cherry picked from commit 5e87eb3)

chunyisong · 2024-01-12T00:16:57Z

So,has this issue been solved ? Or another pr will resolve "TCP deadlock on channel reuse"? @EduPonz @MiguelCompany

EduPonz · 2024-01-12T05:54:55Z

Hi @chunyisong,

It has not been resolved yet, as we are currently undertaking some refactor on the TCP transport that will address this an other instabilities.

* Revert "TCP deadlock on channel reuse (#4099)" (#4181) * Revert "TCP deadlock on channel reuse (#4099)" This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Mark large_data tests as flaky due to TCP Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Add XFAIL_FASTRTPS.list file Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com> Co-authored-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>

* Revert "TCP deadlock on channel reuse (#4099)" This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Mark large_data tests as flaky due to TCP Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com> (cherry picked from commit 5e87eb3) Co-authored-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>

* Revert "TCP deadlock on channel reuse (#4099)" (#4181) * Revert "TCP deadlock on channel reuse (#4099)" This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20055: Mark large_data tests as flaky due to TCP Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com> (cherry picked from commit 5e87eb3) * Methods to configure transport scenarios (#4098) * Refs #20020. Added enumeration for possible builtin transports configuration. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Add method to parse environment variable. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Cleanup RTPSParticipantAttributes.h. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Moved default transports configuration into RTPSParticipantAttributes::setup_transports. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. RTPSParticipantImpl constructor uses private copy of attributes. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Added support for DEFAULTv6. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. SHM transport added before UDP. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Added support for SHM. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Added support for UDPv4. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Added support for UDPv6. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Factor out duplicated code. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Added support for LARGE_DATA. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Added support for LARGE_DATAv6. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020. Added DomainParticipantQos::setup_transports. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #20020: Add constants for builtin transports Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Add XML parser functions for builtin_transports Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: add builtinTransports to .xsd Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: add mock tests needed Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Add builtin transports XML file Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Add new API to tests classes Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Add Blackbox tests Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs 20020: Rename XML file and EOF Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Add unittest test Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com> * Refs #20020: Update versions.md Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Make enum uint16_t Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Add Parsing test Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Apply minor changes Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Remove - in CMakeLists Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Uncrustify Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Fix .xsd and remove unnecesary mock test Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Add implementation for mock test Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Fix windows build Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20020: Uncrustify Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Disable v6 tests for Mac Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> --------- Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> Signed-off-by: elianalf <62831776+elianalf@users.noreply.github.com> Co-authored-by: cferreiragonz <carlosferreira@eprosima.com> Co-authored-by: elianalf <62831776+elianalf@users.noreply.github.com> (cherry picked from commit 8cbd461) # Conflicts: # include/fastdds/dds/domain/qos/DomainParticipantQos.hpp # include/fastdds/rtps/attributes/RTPSParticipantAttributes.h # include/fastrtps/xmlparser/XMLParser.h # resources/xsd/fastRTPS_profiles.xsd # src/cpp/rtps/participant/RTPSParticipantImpl.cpp # src/cpp/rtps/xmlparser/XMLElementParser.cpp # test/unittest/dynamic_types/CMakeLists.txt # test/unittest/statistics/dds/CMakeLists.txt # test/unittest/xmlparser/CMakeLists.txt # versions.md * Fix Conflicts: dpqos Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Fix Conflicts: XMLParser Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Fix Conflicts: RTPSParticipant Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Fix Conflicts: Tests Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Fix Conflicts others Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Cherry-pick c46518643 Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Fix Conflicts: versions.md Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Fix dns filter Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #20055: Separate builtin transports tests into individual cases Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Mark large_data tests as flaky due to TCP Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> --------- Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> Signed-off-by: EduPonz <eduardoponz@eprosima.com> Co-authored-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: cferreiragonz <carlosferreira@eprosima.com>

jepemi force-pushed the hotfix/channel_reuse_deadlock branch from 31f1ad5 to 1264331 Compare December 5, 2023 06:40

jepemi marked this pull request as ready for review December 5, 2023 06:50

EduPonz added this to the v2.13.0 milestone Dec 5, 2023

EduPonz requested a review from MiguelCompany December 5, 2023 08:26

MiguelCompany requested changes Dec 6, 2023

View reviewed changes

test/blackbox/common/TCPReqRepHelloWorldReplier.cpp Outdated Show resolved Hide resolved

test/blackbox/common/TCPReqRepHelloWorldReplier.hpp Outdated Show resolved Hide resolved

jepemi added 3 commits December 11, 2023 08:37

Refs #19939: Channel disabling relocated and OnDataReceived() callbac…

81a0c5b

…k protected Signed-off-by: Jesus Perez <jesusperez@eprosima.com>

Refs #19939: Uncrustify

8db850f

Signed-off-by: Jesus Perez <jesusperez@eprosima.com>

Refs #19939: Apply suggestions

9bdd43f

Signed-off-by: Jesus Perez <jesusperez@eprosima.com>

jepemi force-pushed the hotfix/channel_reuse_deadlock branch from 92ca25c to 9bdd43f Compare December 11, 2023 07:49

MiguelCompany approved these changes Dec 12, 2023

View reviewed changes

MiguelCompany added the ci-pending PR which CI is running label Dec 12, 2023

EduPonz merged commit dd4c434 into master Dec 13, 2023
12 of 14 checks passed

EduPonz deleted the hotfix/channel_reuse_deadlock branch December 13, 2023 06:49

This was referenced Dec 13, 2023

[19939] TCP deadlock on channel reuse (backport #4099) #4127

Merged

[19939] TCP deadlock on channel reuse (backport #4099) #4128

Merged

mergify bot mentioned this pull request Dec 13, 2023

[19939] TCP deadlock on channel reuse (backport #4099) #4129

Merged

mergify bot mentioned this pull request Dec 13, 2023

[19939] TCP deadlock on channel reuse (backport #4099) #4130

Closed

EduPonz mentioned this pull request Dec 20, 2023

Revert "TCP deadlock on channel reuse (#4099)" #4181

Merged

8 tasks

cferreiragonz pushed a commit that referenced this pull request Dec 21, 2023

Revert "TCP deadlock on channel reuse (#4099)"

f7c30c1

This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com>

EduPonz added a commit that referenced this pull request Dec 21, 2023

Revert "TCP deadlock on channel reuse (#4099)"

af60473

This reverts commit dd4c434. Signed-off-by: EduPonz <eduardoponz@eprosima.com>

chunyisong mentioned this pull request Jan 31, 2024

Deadlock appears be caused by write data and discovery entity threads using tcp transport on the writer side. #4203

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[19939] TCP deadlock on channel reuse #4099

[19939] TCP deadlock on channel reuse #4099

jepemi commented Dec 5, 2023 •

edited by EduPonz

Loading

jepemi commented Dec 12, 2023

EduPonz commented Dec 12, 2023

EduPonz commented Dec 12, 2023

EduPonz commented Dec 12, 2023

mergify bot commented Dec 12, 2023 •

edited

Loading

chunyisong commented Jan 12, 2024 •

edited

Loading

EduPonz commented Jan 12, 2024

[19939] TCP deadlock on channel reuse #4099

[19939] TCP deadlock on channel reuse #4099

Conversation

jepemi commented Dec 5, 2023 • edited by EduPonz Loading

Description

Contributor Checklist

Reviewer Checklist

jepemi commented Dec 12, 2023

EduPonz commented Dec 12, 2023

EduPonz commented Dec 12, 2023

EduPonz commented Dec 12, 2023

mergify bot commented Dec 12, 2023 • edited Loading

✅ Backports have been created

chunyisong commented Jan 12, 2024 • edited Loading

EduPonz commented Jan 12, 2024

jepemi commented Dec 5, 2023 •

edited by EduPonz

Loading

mergify bot commented Dec 12, 2023 •

edited

Loading

chunyisong commented Jan 12, 2024 •

edited

Loading