Add a Transport::TCP class for tcp-based communication. #2689

andy31415 · 2020-09-16T18:02:03Z

Problem

CHIP requires the ability to communicate over TCP (for key negotiations, for faster data transfers when supported (e.g. OTA)).

Summary of Changes

Implements a TCP-based transport, with equivalent functionality as the current Transport::UDP.

fixes #2664

…tion oriented transports need disconnection support

…ional

… at least they compile

…es to control the size of those buffers

…receive implementation starting point

… be done

… is frustrating

…but unit tests pass so TCP is maybe ok

BroderickCarlin · 2020-09-16T21:30:27Z

src/transport/PeerAddress.h

@@ -48,7 +48,7 @@ enum class Type
    kUndefined,
    kUdp,
    kBle,
-    // More constants to be added later, such as TCP


Should we maintain this comment or is TCP the last constant we plan to add?

I am not aware of anything else we could add as chip spec. I believe a comment would be 'add more stuff here if needed' however that seems redundant as any enum would be 'add more if needed'.

src/transport/Base.h

src/transport/TCP.h

src/transport/TCP.cpp

src/transport/TCP.h

bzbarsky-apple · 2020-09-16T22:21:05Z

src/transport/TCP.h

+    // Called when a connection has been completed
+    static void OnConnectionComplete(Inet::TCPEndPoint * endPoint, INET_ERROR err);
+
+    // Called when a connection has been closed


Might be worth documenting how this differs from OnPeerClosed...

I updated this mentioning this originates from TCPEndPoint.
That class has two callbacks. I am actually unsure why - peerclosed is on read 0 (peer closed) however I am unclear how this one is called (OnClose and OnAbort actually null-out the callbacks with the comment 'application called close explicitly')

src/transport/Tuple.h

src/transport/tests/TestTCP.cpp

bzbarsky-apple · 2020-09-16T22:30:52Z

src/transport/tests/TestTCP.cpp

+    err = tcp.SendMessage(header, Transport::PeerAddress::TCP(addr), buffer);
+    if (err == System::MapErrorPOSIX(EADDRNOTAVAIL))
+    {
+        // TODO: the underlying system does not support IPV6. This early return should


Please file an issue tracking this.

This is a copy and paste from TestUDP ... at that time we had errors in CI, which should already have tracking issues.
I will update the wording.

Added an issue after all, since I believe the original issue for CI is already addressed in a curent PR which likely will not touch TestTCP/UDP.

Opened issue is #2698 and is now referenced in the TODO.

andy31415 · 2020-09-17T13:26:48Z

@bzbarsky-apple I have addressed all comments. Will double check on freeing paths ... I wish we had some 'smart pointer' like constructs instead of raw pointers. Tracking buffer usage manually seems error prone to me. Maybe topic for the future.

andy31415 · 2020-09-17T13:51:06Z

@bzbarsky-apple I have addressed all comments. Will double check on freeing paths ... I wish we had some 'smart pointer' like constructs instead of raw pointers. Tracking buffer usage manually seems error prone to me. Maybe topic for the future.

SecureSession does indeed Free the passed in buffer, so right now all our paths seem to behave correctly (after my updates to the test).

…tion

…or internal logic errors

mspang · 2020-09-17T14:41:32Z

src/transport/PeerAddress.h

    {
-        return (mTransportType == other.mTransportType) && (mIPAddress == other.mIPAddress) && (mPort == other.mPort);
+        return (mTransportType == other.mTransportType) && (mIPAddress == other.mIPAddress) && (mPort == other.mPort) &&
+            ((mInterface == INET_NULL_INTERFACEID) || (other.mInterface == INET_NULL_INTERFACEID) ||


I think this field should be matched exactly.

We shouldn't understand equality of addresses to mean "could these addresses send to the same destination" and we couldn't answer that question in general.

It should just mean "these addresses are identical", including zone identifier.

Weaker matches can be separate functions, not equality.

mspang · 2020-09-17T14:47:45Z

src/transport/TCP.h

+        mActiveConnectionsSize(bufferSize), mPendingPackets(packetBuffers), mPendingPacketsSize(packetsBuffersSize)
+    {
+        PendingPacket emptyPending{
+            .peerAddress  = PeerAddress::Uninitialized(),


The reason it compiles it's been a GNU extension for ages (and we've been compiling with GNU extensions).

…CP as it is more than just peer data but rather a complete state that can never be recovered on the server side

woody-apple · 2020-09-17T18:43:32Z

@saurabhst ?

bzbarsky-apple · 2020-09-17T19:19:02Z

I wish we had some 'smart pointer' like constructs instead of raw pointers.

Yes, indeed. I filed #2707

bzbarsky-apple · 2020-09-17T18:12:55Z

src/transport/TCP.cpp

+            VerifyOrExit(buffer->DataLength() >= messageSize, err = CHIP_ERROR_INTERNAL);
+
+            err = ProcessSingleMessageFromBufferHead(peerAddress, buffer, messageSize);
+            buffer->ConsumeHead(messageSize);


Hmm. It occurs to me: Should this call to ConsumeHead still happen even if ProcessSingleMessageFromBufferHead failed? We're about to push this buffer back to the endpoint, no? I would not think we want to consume data from it... Though maybe the idea is to push back only the data after the failed message? In that case that should be clearly documented.

I thought to keep it because I alrady skipped over the size. If processsinglemessage fails on a buffer, there seems to be reasonable expectation that it will fail in the future as well. This also guarantees that we empty buffers in time.

I.e. I think this is correct and it would be more work to not consume on error.

OK. That seems fine, but worth a comment explaining why we are doing what we are doing.

bzbarsky-apple · 2020-09-17T18:42:38Z

src/transport/TCP.cpp

+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(Inet, "Connection complete encountered an error: %s", ErrorStr(err));
+        endPoint->Free();


Yes, I agree with making the endpoint count stuff better in a followup. For now, file an issue and add a TODO comment at the member declaration pointing to the issue?

bzbarsky-apple · 2020-09-17T18:43:49Z

src/transport/TCP.h

+        mActiveConnectionsSize(bufferSize), mPendingPackets(packetBuffers), mPendingPacketsSize(packetsBuffersSize)
+    {
+        PendingPacket emptyPending{
+            .peerAddress  = PeerAddress::Uninitialized(),


I guess it's ok until someone complains that it doesn't compile for their hardware. ;)

bzbarsky-apple · 2020-09-17T19:19:08Z

src/transport/TCP.h

+                                                  uint16_t messageSize);
+
+    // Callback handler for TCPEndPoint. TCP message receive handler.
+    static void OnTcpReceive(Inet::TCPEndPoint * endPoint, System::PacketBuffer * buffer);


This should probably document ownership semantics too...

Should be handled by TCPEndPoint. That documents it as:

* A data reception event handler must acknowledge data processed using * the \c AckReceive method. The \c Free method on the data buffer must * also be invoked unless the \c PutBackReceivedData is used instead

I think we should not try to duplicate docs lest they become out of sync.

OK, sounds good! Could add a pointer to the TCPEndPoint documentation from here.

andy31415 · 2020-09-17T20:09:34Z

Created #2708 for better active count usage.

andy31415 added 25 commits September 15, 2020 09:49

Add Disconnect call support in transport base and tuple, since connec…

a3ece7d

…tion oriented transports need disconnection support

Remove message send port from UDP - peer address already contains a port

5a64e0f

Add tcp transport type to peer addresses

80c881b

Create a TCP header and cpp files that compile with gn. Not yet funct…

aace8a8

…ional

Added TCP tests. They fail since no implementation available, however…

1d6c6b9

… at least they compile

Started to rename more stuff from UDP to TCP in tests

57cdb9c

Add a buffer of active connections within tcp connection. Use templat…

5c98783

…es to control the size of those buffers

Added an array of "Pending packets" for the TCP interface

4fefbb2

Free pending packet buffers in the TCP destructor

a6c8f9e

Start adding ability to enqueu and start connections over TCP

a46519e

More work on connections

bc911d6

Setup for data receive

443add7

Setup for data receive

8e7f6ac

Seeing TCP data being received

be7ad2c

Prepare for message receiving. CHIPConnection logic can be used as a …

1a1425f

…receive implementation starting point

Added logic for message receiving and parsing

ef4a42c

Unit tests now pass, however connections are not properly closed yet

10fc27e

Unit tests pass

3d7a4c7

Add ability to close active connections and wait for them to actually…

517235a

… be done

Work towards being able to close active connections

ed3cb92

Add close notification handling and peer close callback

6b66de4

One end of the connection now is cleared. Not both ends though, which…

8b9d993

… is frustrating

Update accept error handling: nothing we can do except log

be3d830

Slight comment update

0e4819f

Update comment on what causes time wait in connections. Frustrating, …

628359f

…but unit tests pass so TCP is maybe ok

andy31415 requested review from BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248 and jelderton as code owners September 16, 2020 18:02

BroderickCarlin reviewed Sep 16, 2020

View reviewed changes

bzbarsky-apple requested changes Sep 16, 2020

View reviewed changes

Code review updates

65b37fd

andy31415 requested a review from woody-apple as a code owner September 17, 2020 13:24

andy31415 added 4 commits September 17, 2020 10:44

Fix typo

c67cb08

More code review updates: add a free on error and update file descrip…

8083d21

…tion

More code review updates: ensure endpoint count is updated, add log f…

287b5cd

…or internal logic errors

Update one more place for used endpoint count decreasing

a8637f0

mspang approved these changes Sep 17, 2020

View reviewed changes

andy31415 added 7 commits September 17, 2020 11:03

Call disconnect on session expiry

1b60cbc

Updated comment regarding expiry. Expiry is especially critical for T…

3b27f33

…CP as it is more than just peer data but rather a complete state that can never be recovered on the server side

Fix typo

7f5dfa4

Code review updates

ab3202a

More comment updates

9d9e445

More comment updates

73e12e8

Make peer address equality strict on interface id

da64988

andy31415 requested review from bzbarsky-apple and BroderickCarlin September 17, 2020 17:20

woody-apple approved these changes Sep 17, 2020

View reviewed changes

saurabhst approved these changes Sep 17, 2020

View reviewed changes

bzbarsky-apple approved these changes Sep 17, 2020

View reviewed changes

andy31415 merged commit d1f0a37 into project-chip:master Sep 17, 2020

andy31415 mentioned this pull request Sep 17, 2020

Documentation cleanup for TCP transport #2709

Merged

bzbarsky-apple mentioned this pull request Sep 18, 2020

TestTCP fails on Mac #2721

Closed

andy31415 deleted the 01_implement_tcp_transport branch October 9, 2020 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Transport::TCP class for tcp-based communication. #2689

Add a Transport::TCP class for tcp-based communication. #2689

andy31415 commented Sep 16, 2020

BroderickCarlin Sep 16, 2020

andy31415 Sep 17, 2020

bzbarsky-apple Sep 16, 2020

andy31415 Sep 17, 2020

bzbarsky-apple Sep 16, 2020

andy31415 Sep 17, 2020

andy31415 Sep 17, 2020

andy31415 commented Sep 17, 2020 •

edited

Loading

andy31415 commented Sep 17, 2020

mspang Sep 17, 2020

mspang Sep 17, 2020

woody-apple commented Sep 17, 2020

bzbarsky-apple commented Sep 17, 2020

bzbarsky-apple Sep 17, 2020

andy31415 Sep 17, 2020

bzbarsky-apple Sep 17, 2020

bzbarsky-apple Sep 17, 2020

bzbarsky-apple Sep 17, 2020

bzbarsky-apple Sep 17, 2020

andy31415 Sep 17, 2020

bzbarsky-apple Sep 17, 2020

andy31415 commented Sep 17, 2020

Add a Transport::TCP class for tcp-based communication. #2689

Add a Transport::TCP class for tcp-based communication. #2689

Conversation

andy31415 commented Sep 16, 2020

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy31415 commented Sep 17, 2020 • edited Loading

andy31415 commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woody-apple commented Sep 17, 2020

bzbarsky-apple commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy31415 commented Sep 17, 2020

andy31415 commented Sep 17, 2020 •

edited

Loading