TCP transport by JuanCarlos-Arce · Pull Request #251 · eProsima/Fast-DDS

JuanCarlos-Arce · 2018-09-03T13:57:59Z

Implementation of a new transport layer to manage TCP communication. These new transports are connection oriented, and they manage internally the negotiation and the reconnection if the communication is lost. The transport layer has been restructured to fit with the needs of the TCP communication and to ease the management of the sockets.

This feature includes:

New TCPv4Transport and TCPv6Transport classes.
New TransportInterface class that unifies the common code of the transports.
Create UDPSocketInfo and TCPSocketInfo to encapsulate the management of the sockets and their resources.
Create a new project "HelloWorldExampleTCP"
Remove the attribute "DefaultSendPort" from participants.

MiguelCompany

Partial review: examples and include was covered.

General feedback

Some files contain TAB characters. We use 4 spaces for indentation.
Some files on the examples seem to have been generated with an old version of fastrtpsgen. Please fix this

Design feedback
It seems the transport layer is using information from the upper layer (participant guid is passed to the transport layer, ReceiverResource is processing messages). This should not be the case. I understand that on TCP we can receive an RTPSMessage that should be sent to several participants. On UDP was easy, as multiple sockets can be listening on the same port, and all of them will receive a copy of the message. I understand that we may need a way to pass the received message to several entities of the uper layer, but that sould be done without knowledge of the content of the messages.

MiguelCompany · 2018-09-04T10:18:55Z

examples/C++/Benchmark/README.txt

+
+In the first one launch: 'BenchMark publisher tcp' or 'BenchMark publisher udp' (or HelloWorldExample.exe publisher on windows).
+In the second one: 'HelloWorldExample subscriber tcp' or 'HelloWorldExample subscriber udp'
+


Seems you forgot to change some HelloWorldExample to BenchMark

MiguelCompany · 2018-09-04T12:45:56Z

examples/C++/ClientServerTest/main_ClientServerTestFastRTPS.cpp

 using std::cout;
 using std::endl;
-
+/*


I suppose this is to remove the 'unused variable' warning. I think it's better to remove the code completely.

MiguelCompany · 2018-09-04T12:48:32Z

examples/C++/ClientServerTest/EprosimaClient.cpp

 	//CREATE RTPSParticipant
 	ParticipantAttributes PParam;
-	PParam.rtps.defaultSendPort = 10042;
+	//PParam.rtps.defaultSendPort = 10042; // TODO Create transport?


As defaultSendPort is being removed, and I don't like TODOs on examples, please remove the whole line

MiguelCompany · 2018-09-04T12:48:52Z

examples/C++/ClientServerTest/EprosimaServer.cpp

 	//CREATE RTPSParticipant
 	ParticipantAttributes PParam;
-	PParam.rtps.defaultSendPort = 10042;
+	//PParam.rtps.defaultSendPort = 10042; // TODO Create transport?


As defaultSendPort is being removed, and I don't like TODOs on examples, please remove the whole line

MiguelCompany · 2018-09-04T12:59:05Z

examples/C++/HelloWorldExample/HelloWorldPublisher.cpp

 }

-void HelloWorldPublisher::run(uint32_t samples)
+void HelloWorldPublisher::runThread(uint32_t samples, uint32_t sleep)


Why are we complicating the most basic example with the creation of an additional thread?

This additional thread allows to stop the publisher pressing enter. This is useful when HelloWorldExample is executed as publisher without a limit of samples.

MiguelCompany · 2018-09-07T09:18:35Z

include/fastrtps/rtps/network/ReceiverResource.h

 namespace fastrtps{
 namespace rtps{

+class RTPSWriter;


I don't think this class should have knowledge of the upper layer. If we need to pass a received message to several entities, we may register more than one MessageReceiver on the same ReceiverResource.

MiguelCompany · 2018-09-07T09:33:40Z

include/fastrtps/rtps/network/SenderResource.h

 namespace fastrtps{
 namespace rtps{

+class RTPSParticipantImpl;


SenderResource should only know about locators and data, not about participants and guids

MiguelCompany · 2018-09-07T09:35:17Z

include/fastrtps/transport/ChannelResource.h

+namespace fastrtps{
+namespace rtps{
+
+class ChannelResource


The purpose of this class should be documented

MiguelCompany · 2018-09-07T09:36:23Z

include/fastrtps/transport/ChannelResource.h

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef CHANNEL_RESOURCE_INFO_


Define doesn't match filename

MiguelCompany · 2018-09-07T10:04:22Z

include/fastrtps/transport/TransportInterface.h

+protected:

-    uint32_t maxMessageSize;
+    //! Participant GUID prefix.


Why should a transport need the GUID prefix?

… it into the socket infos.

… TCP negotiation.

…the header file.

…ture connections.

- Update TCP transport to fix an issue opening output channels over an input socket.

…the hello world initialization for UDP.

…s union types yet. OpenLogicalPort negociation seems to work fine, redirection maybe not.

…r Android Devices. - Update project configuration to build fastrtps with fastcdr in Windows

richiware · 2018-09-06T14:35:20Z

src/cpp/rtps/participant/RTPSParticipantImpl.cpp

+            break;
+        case LOCATOR_KIND_TCPv4:
+        case LOCATOR_KIND_TCPv6:
+            //TODO: Check Physical <-> Logical Port


What this TODO means?

richiware · 2018-09-06T14:36:32Z

include/fastrtps/rtps/common/Locator.h

+#define LOCATOR_KIND_TCPv4 4
+#define LOCATOR_KIND_TCPv6 8

+// TODO Make this class a base class for each kind of locator?


What this TODO means?

richiware · 2018-09-10T05:29:10Z

src/cpp/rtps/participant/RTPSParticipantImpl.cpp

+                    m_att.port.offsetd3 +
+                    m_att.port.participantIDGain*m_att.participantID), true);
+            }
+            if (loc.get_physical_port() == 0)


This if can be true when transport is UDP and change the value of the port, which was set already before.

Old review.

richiware changed the title ~~Feature/tcp transport~~ TCP transport Sep 3, 2018

JuanCarlos-Arce force-pushed the feature/tcp-transport branch from 7bafd5f to 5c067c7 Compare September 3, 2018 14:24

richiware requested review from MiguelCompany and richiware September 3, 2018 14:25

MiguelCompany added enhancement needs-review PR that is ready to be reviewed labels Sep 4, 2018

richiware mentioned this pull request Sep 5, 2018

Use the interface whitelist to bind multicast input sockets and filte… [4559] #231

Closed

richiware changed the title ~~TCP transport~~ TCP transport (#3) Sep 6, 2018

richiware changed the title ~~TCP transport (#3)~~ TCP transport Sep 6, 2018

MiguelCompany previously requested changes Sep 7, 2018

View reviewed changes

MiguelCompany force-pushed the feature/tcp-transport branch from dd7e6b9 to 34630fd Compare October 1, 2018 11:24

Juan Carlos Arceredillo and others added 19 commits October 1, 2018 16:23

Refs #2920 - Separate the cdr buffer from the message receives to set…

8af2f34

… it into the socket infos.

Refs #2920 - Update the binding method for remote locators during the…

f914493

… TCP negotiation.

Refs #2920 - Create socketinfo.cpp with the implemented methods from …

5c9ef79

…the header file.

Refs #2920 Updated receivers management to store them in a map for fu…

f434f8e

…ture connections.

Refs #2920 Fix UDP refactoring issues

baaf6b1

Refs #2928 - Add TCP types to the XML Parser

05d22f8

- Update TCP transport to fix an issue opening output channels over an input socket.

Refs #2928 - Remove the shared pointers from the Socket Infos

84aab63

Refs #2928 - Remove shared pointers from several mutex on socket info

001e5fb

Refs #2928 - Remove the shared pointers for TCP acceptors and update …

c57a222

…the hello world initialization for UDP.

Refs #2926

cf896cd

Refs #2927 Only BindConnection adapted

9622c4e

Refs #2927 Payload usage

32ead07

Refs #2927 Payload not used in headers. Size fixed.

16f3314

Refs #2927 TCP Control Messages uses Cdr, but they are not using idl'…

a63633f

…s union types yet. OpenLogicalPort negociation seems to work fine, redirection maybe not.

Refs #2928 - Adapt TCPv4 Transport without the ASIO_HAS_MOVE Macro fo…

44b7471

…r Android Devices. - Update project configuration to build fastrtps with fastcdr in Windows

Refs #2928 - Fix Compilation issue.

e107303

Refs #2927 Using Payload directly.

1faf141

Refs #2927 Changed how buffer size is allocated.

49c4e5e

Refs #2928 - Fix compilation warnings

4e6e0e8

Luis Gasco and others added 17 commits October 1, 2018 16:27

Refs #3413. Fixed double free.

5027d30

Refs #3413. Fixed double free due to bug.

0c71650

Refs #3413. Wait for negotiation restored.

9a3b5da

Refs #3413. Fixed warnings.

ebbd528

Refs #3413. Minor improvements

52bf8fa

Refs #3413. Allow TCP blackbox tests to run in parallel

dbeef0b

Refs #3413. Each transport adds its own metatraffic locators.

85c8249

Refs #3413. Fixed unused parameters warnings.

0cdb12f

Refs #3413. Fix bug setting logical default port for metatraffic.

21f8ab2

Refs #3413. Each transport adds its own default unicast locators.

52d3a78

Refs #3413. New HelloWorld TCP Configuration.

917f44a

Refs #3713. Fixing TCP BlackBox tests.

6a551d7

Refs #3713. Fixing TCP std::remove bad use.

2c17158

Refs #3413. Fixing errors detected by unit tests.

57fd8e5

Refs #3413. Fixing valgrind errors

44b1c28

Refs #3413. Picked changes from develop

6f6e2bf

Refs #3413. Fixed valgrind errors.

c26c823

MiguelCompany force-pushed the feature/tcp-transport branch from 35f9db6 to c26c823 Compare October 2, 2018 05:01

MiguelCompany assigned richiware Oct 2, 2018

Luis Gasco and others added 6 commits October 9, 2018 12:28

Refs #3413. Fixed more valgrind errors.

60fc1c9

Refs #3413. Removed unused arguments.

5e9fd05

Refs #3413. Removed warnings.

71043b2

Refs #3413. Improve tcp blackbox tests.

9926b59

Refs #3413. Added retries if not logical port openend.

899ed14

Refs #3413. Removed unnecessary sleep.

3e17ba3

richiware approved these changes Oct 11, 2018

View reviewed changes

richiware merged commit cf247a5 into develop Oct 11, 2018

richiware deleted the feature/tcp-transport branch October 11, 2018 13:21

MiguelCompany mentioned this pull request Nov 7, 2018

Refs #3443. Added equality operator for attributes. #271

Closed


		In the first one launch: 'BenchMark publisher tcp' or 'BenchMark publisher udp' (or HelloWorldExample.exe publisher on windows).
		In the second one: 'HelloWorldExample subscriber tcp' or 'HelloWorldExample subscriber udp'

Conversation

JuanCarlos-Arce commented Sep 3, 2018

Uh oh!

MiguelCompany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants