Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[20313] Allow single listening port on TCP #4348

Merged
merged 4 commits into from
Mar 1, 2024
Merged

[20313] Allow single listening port on TCP #4348

merged 4 commits into from
Mar 1, 2024

Conversation

jepemi
Copy link
Contributor

@jepemi jepemi commented Feb 1, 2024 •
edited by MiguelCompany
Loading

Description

When several listening ports are added to the TCP transport, the data communication does not work or, if it does, it is in an undesired way. Servers create an acceptor on each of the listening ports. Since the server locators sent in the discovery phase only include the first added listening port, clients connected to another of the listening ports never find the resource that their initial peer initially created and end up creating a new channel for the locators obtained from the discovery data. If this new resource manages to connect, a new thread is created on both sides.

This PR limits the amount of listening ports to one, which effectively will only create an acceptor on the first indicated listening port. If more listening ports are passed, those are ignored.

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
  • 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).
  • New feature has been documented/Current behavior is correctly described in the documentation.
    Related documentation PR: [20313] Allow single listening port on TCP Fast-DDS-docs#717
  • Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@jepemi jepemi added no-test Skip CI tests if PR marked with this label skip-ci Automatically pass CI and removed no-test Skip CI tests if PR marked with this label labels Feb 1, 2024
@jepemi jepemi force-pushed the bugfix/tcp_fix_multiple_listening_ports branch from 19ee370 to 2fd7a0e Compare February 1, 2024 16:08
@jepemi jepemi marked this pull request as ready for review February 1, 2024 16:10
@jepemi jepemi removed the skip-ci Automatically pass CI label Feb 1, 2024
@jepemi jepemi changed the base branch from master to bugfix/transport_sanitizer_event February 2, 2024 07:11
@cferreiragonz cferreiragonz added the needs-review PR that is ready to be reviewed label Feb 6, 2024
@EduPonz EduPonz added this to the v2.13.3 milestone Feb 6, 2024
@jepemi jepemi force-pushed the bugfix/transport_sanitizer_event branch from 07bfc00 to b4a0291 Compare February 9, 2024 11:43
@jepemi jepemi removed the needs-review PR that is ready to be reviewed label Feb 19, 2024
@jepemi jepemi force-pushed the bugfix/tcp_fix_multiple_listening_ports branch from 2fd7a0e to 17a6d47 Compare February 19, 2024 09:54
@jepemi jepemi changed the base branch from bugfix/transport_sanitizer_event to bugfix/wifi-eth-whitelist February 19, 2024 09:55
@jepemi jepemi closed this Feb 19, 2024
@jepemi jepemi reopened this Feb 19, 2024
@jepemi jepemi added the needs-review PR that is ready to be reviewed label Feb 19, 2024
@JesusPoderoso JesusPoderoso added to-do and removed needs-review PR that is ready to be reviewed labels Feb 20, 2024
@EduPonz EduPonz added needs-review PR that is ready to be reviewed needs rebase and removed to-do labels Feb 20, 2024
@jepemi jepemi force-pushed the bugfix/tcp_fix_multiple_listening_ports branch from 17a6d47 to 2897ae3 Compare February 20, 2024 12:02
MiguelCompany
MiguelCompany previously approved these changes Feb 20, 2024
View reviewed changes
@MiguelCompany MiguelCompany added ci-pending PR which CI is running and removed needs-review PR that is ready to be reviewed labels Feb 20, 2024
@MiguelCompany MiguelCompany modified the milestones: v2.13.3, v2.14.0 Feb 20, 2024
@MiguelCompany
Copy link
Member

Moved to 2.14, since this changes behavior

@MiguelCompany MiguelCompany changed the title [20313] TCP fix for multiple listening ports [20313] Allow single listening port on TCP Feb 20, 2024
Base automatically changed from bugfix/wifi-eth-whitelist to master February 21, 2024 06:44
@MiguelCompany MiguelCompany dismissed their stale review February 21, 2024 06:44

The base branch was changed.

@MiguelCompany
Copy link
Member

@jepemi This has some conflicts that need to be resolved before reviewing

@MiguelCompany MiguelCompany added needs-review PR that is ready to be reviewed and removed ci-pending PR which CI is running labels Feb 28, 2024
@jepemi jepemi force-pushed the bugfix/tcp_fix_multiple_listening_ports branch from 2897ae3 to 6e11055 Compare February 28, 2024 09:57
MiguelCompany
MiguelCompany approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI

MiguelCompany
MiguelCompany requested changes Feb 28, 2024
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix warnings on Windows (signed / unsigned comparisons in the test)

MiguelCompany
MiguelCompany previously approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI

@MiguelCompany MiguelCompany added ci-pending PR which CI is running and removed needs-review PR that is ready to be reviewed labels Feb 28, 2024
@EduPonz
Copy link

EduPonz commented Feb 29, 2024

@richiprosima please test this

@JesusPoderoso
Copy link
Contributor

Although Jenkins Mac CI has not been run, Mac GitHub CI passes.
This PR is Ready to merge!

@JesusPoderoso JesusPoderoso added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Mar 1, 2024
@MiguelCompany MiguelCompany added the doc-pending Issue or PR which is pending to be documented label Mar 1, 2024
@MiguelCompany
Copy link
Member

@jepemi This is ready to merge, but I think a documentation PR describing the limitation is necessary

@jepemi
Refs #20313: Check initial peers before connect type channel creation…
f9ed933 
… & fix fill unicast locators

Signed-off-by: Jesus Perez <jesusperez@eprosima.com>
@jepemi
Refs #20313: PR refactor
3ff09b2 
Signed-off-by: Jesus Perez <jesusperez@eprosima.com>
@jepemi
Refs #20313: Fix windows tests
b1e28a3 
Signed-off-by: Jesus Perez <jesusperez@eprosima.com>
@jepemi
Refs #20313: Modify autofill_port test
cdf6c46 
Signed-off-by: Jesus Perez <jesusperez@eprosima.com>
@jepemi jepemi force-pushed the bugfix/tcp_fix_multiple_listening_ports branch from e20c1cc to cdf6c46 Compare March 1, 2024 12:05
@jepemi jepemi mentioned this pull request Mar 1, 2024
Merged
6 tasks
@MiguelCompany MiguelCompany removed the doc-pending Issue or PR which is pending to be documented label Mar 1, 2024
@MiguelCompany MiguelCompany merged commit 8766e52 into master Mar 1, 2024
9 of 10 checks passed
@MiguelCompany MiguelCompany deleted the bugfix/tcp_fix_multiple_listening_ports branch March 1, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.14.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jepemi @MiguelCompany @EduPonz @JesusPoderoso @cferreiragonz