TCP Transport Rewrite #946

shoamano83 · 2018-04-26T13:33:44Z

This PR is a part of implementation of #900. It also fixes issue #911.

This PR is ready for review.

Risk

This PR makes minor API changes: addition of NSNotification.

Testing Plan

Unit tests are included in the PR.

Summary

This PR rewrites TCP transport as requested by project maintainer (see smartdevicelink/sdl_evolution#405 (comment)).

The project maintainer suggested to use NSURLSessionStreamTask API and I had implemented with it. Then I found a few issues. 1) it didn't work with my iOS 9.0.2 phone, and 2) the API doesn't seem to provide "TCP connected" event. After a short offline discussion with the project maintainer, I decided to use CFNetwork API instead.

Changelog

Breaking Changes

None.

Enhancements

TCP transport is updated to use CFNetwork and NSStream API. The run loop runs on dedicated thread.
TCP connection errors are now notified to the app through NSNotification.
New delegates are added in TCPTransportDelegate and SDLProtocolListener. (The header files were once public, now are private.)

Bug Fixes

None.

CLA

I have signed the CLA

shoamano83 · 2018-04-26T13:36:40Z

SmartDeviceLink/SDLProtocolListener.h

@@ -23,6 +23,7 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)onProtocolOpened;
 - (void)onProtocolClosed;
 - (void)onError:(NSString *)info exception:(NSException *)e;
+- (void)onTransportError:(NSError *)error;


The "onError:exception:" delegate (one line above) isn't and hasn't been used anywhere in the project. We might want to remove it to avoid confusion.

shoamano83 · 2018-04-26T13:44:06Z

A few notes:

This PR still lacks retry logic in the case of connection error. I'm wondering if we should add it, since TCP connection errors are mostly caused by wrong IP address and/or port number and reconnection doesn't usually make sense in such cases.
Please feel free to suggest better naming of the new delegates and error enums.

Thanks!

If a TCP connection is refused or timed out, onError is notified from the transport. Then an error will be delivered to the app through NSNotification.

shoamano83 · 2018-07-05T07:37:08Z

I have rebased the commits onto latest develop branch and resolved conflicts.

joeljfischer · 2018-07-24T14:26:30Z

SmartDeviceLink/SDLError.h

@@ -65,6 +66,13 @@ extern SDLErrorDomain *const SDLErrorDomainChoiceSetManager;
 + (NSError *)sdl_choiceSetManager_choiceDeletionFailed:(NSDictionary *)userInfo;
 + (NSError *)sdl_choiceSetManager_choiceUploadFailed:(NSDictionary *)userInfo;

+#pragma mark Transport
+
+ (NSError *)sdl_transport_OthersError;


Please rename sdl_transport_unknownError

Done in commit 72e4cb3.

joeljfischer · 2018-07-24T14:32:12Z

SmartDeviceLink/SDLErrorConstants.h

+    /**
+     *  Connection cannot be established due to a reason not listed here.
+     */
+    SDLTransportErrorOthers = -1,


SDLTransportErrorUnknown

Done in commit 72e4cb3.

joeljfischer · 2018-07-24T15:00:05Z

SmartDeviceLink/SDLTCPTransport.h

@@ -27,6 +25,7 @@ NS_ASSUME_NONNULL_BEGIN
 *  The port number of Core
 */
 @property (strong, nonatomic) NSString *portNumber;
+@property (nonatomic, assign) NSUInteger receiveBufferSize;


Why is this public facing, can it just be in the private interface?

Yes it should be private, fixed with commit 4aa7820.

joeljfischer · 2018-07-24T15:21:36Z

SmartDeviceLink/SDLTCPTransport.m

    [self disconnect];
 }

+#pragma mark - SDLAbstractTransport methods


There is no more SDLAbstractTransport

Renamed with commit 72e4cb3.

joeljfischer · 2018-07-24T15:42:02Z

SmartDeviceLink/SDLTCPTransport.m

+    }
+
+    self.ioThread = [[NSThread alloc] initWithTarget:self selector:@selector(sdl_tcpTransportEventLoop) object:nil];
+    [self.ioThread setName:TCPIOThreadName];


self.ioThread.name = TCPIOThreadName;

Fixed in commit 72e4cb3.

joeljfischer · 2018-07-24T18:19:40Z

SmartDeviceLink/SDLTCPTransport.m

+            [self.delegate onError:error];
+            self.transportErrorNotified = YES;
+        } else {
+            SDLLogW(@"Unhandled stream error! %@", stream.streamError);


Should this be an error log?

Done in commit 4aa7820.

joeljfischer · 2018-07-24T18:19:53Z

SmartDeviceLink/SDLTCPTransport.m

+            // of NSPOSIXErrorDomain are actually errno values.
+            NSError *error;
+            switch (stream.streamError.code) {
+                case ECONNREFUSED:


Please use braces as noted in the comment above

Done in commit 72e4cb3.

joeljfischer · 2018-07-24T18:21:48Z

SmartDeviceLink/SDLTCPTransport.m

+                    error = [NSError sdl_transport_OthersError];
+                    break;
+            }
+            [self.delegate onError:error];


Question: All the errors here are errors causing us to not be able to connect, should we be checking anything here to make sure we're disconnected?

Sorry, I didn't catch your question. Are you suggesting that we should verify stream.streamStatus property here and make sure that the stream is closed?

Well, once sdl_onStreamError: is triggered, the ioThread is cancelled and eventually sdl_teardownStream: will be called for both streams. So I think there may not be much point adding extra logic inside sdl_onStreamError:.

joeljfischer · 2018-07-24T18:23:39Z

SmartDeviceLink/SDLTCPTransport.m

    }
 }

+- (void)sdl_doNothing {
+}


Just one line - (void)sdl_doNothing {}

Done in commit 72e4cb3.

joeljfischer · 2018-07-24T18:26:16Z

SmartDeviceLinkTests/TransportSpecs/SDLTCPTransportSpec.m

+- (void)onClientError;
+@end
+
+@interface TestTCPServer : NSObject {


Please put this class in its own file(s), similar to how the TestConnectionManager is

OK, done in commit 713f409.

- Rename an error enum - Update / remove comments, pragmas and logs - Resolve styling issues

- Change receiveBufferSize to private - Remove `default` case from switch statement - Simplify some if statements - Update logs

- Use NSAssert to check a flag that should be always false

Conflicts: SmartDeviceLink-iOS.xcodeproj/project.pbxproj

shoamano83 · 2018-08-02T02:00:57Z

Hi @joeljfischer , I think I resolved all of your review comments. I also merged latest develop branch to get rid of the merge conflicts. Please kindly review again when you have time.

joeljfischer · 2018-08-02T15:45:16Z

SmartDeviceLink/SDLTCPTransport.m

-        gethostname(localhost, sizeof localhost);
-        hostname = (const char *)&localhost;
+#pragma mark - Stream event handlers
+// these methods run only on the I/O thread (i.e. invoked from the run loop)


Should there be an assert statement in each method making sure they're running on the correct thread?

OK, added in commit c1883ff.

joeljfischer · 2018-08-02T17:34:53Z

SmartDeviceLink/SDLTCPTransport.m

-        fprintf(stderr, "getaddrinfo error: %s\n", gai_strerror(status));
-        return (-1);
+    NSData *data = [NSData dataWithBytesNoCopy:buffer length:(NSUInteger)readBytes freeWhenDone:YES];
+    [self.delegate onDataReceived:data];


This delegate will be called on the io thread. Should we dispatch to a different queue first or something?

I followed the design of iAP transport where onDataReceived is invoked from its I/O thread. I don't think we need to dispatch to another queue.

- Add assertions in I/O callback methods

shoamano83 commented Apr 26, 2018

View reviewed changes

joeljfischer changed the title ~~Feature/rewrite tcp transport~~ TCP Transport Rewrite Apr 26, 2018

joeljfischer added enhancement proposal Accepted SDL Evolution Proposal labels Apr 26, 2018

joeljfischer added this to the 6.1.0 milestone May 24, 2018

shoamano83 added 2 commits July 3, 2018 15:02

Rewrite TCP transport using CFNetwork API

5fc4e5b

Add notification for TCP connection errors

6bf797f

If a TCP connection is refused or timed out, onError is notified from the transport. Then an error will be delivered to the app through NSNotification.

shoamano83 force-pushed the feature/rewrite_tcp_transport branch from 5cf9017 to 6bf797f Compare July 5, 2018 07:36

joeljfischer suggested changes Jul 24, 2018

View reviewed changes

shoamano83 added 6 commits July 27, 2018 12:03

Reflect review comments

72e4cb3

- Rename an error enum - Update / remove comments, pragmas and logs - Resolve styling issues

Reflect review comments

4aa7820

- Change receiveBufferSize to private - Remove `default` case from switch statement - Simplify some if statements - Update logs

Reflect review comments

3e34135

- Use NSAssert to check a flag that should be always false

Move TestTCPServer into dedicated files

713f409

Merge branch 'develop' into feature/rewrite_tcp_transport

d4185a2

Conflicts: SmartDeviceLink-iOS.xcodeproj/project.pbxproj

Merge branch 'develop' into feature/rewrite_tcp_transport

253d166

Conflicts: SmartDeviceLink-iOS.xcodeproj/project.pbxproj

joeljfischer suggested changes Aug 3, 2018

View reviewed changes

Reflect review comment

c1883ff

- Add assertions in I/O callback methods

joeljfischer approved these changes Aug 14, 2018

View reviewed changes

joeljfischer merged commit ef0cfa3 into smartdevicelink:develop Aug 14, 2018

joeljfischer mentioned this pull request Aug 14, 2018

v6.1.0 Release #999

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TCP Transport Rewrite #946

TCP Transport Rewrite #946

shoamano83 commented Apr 26, 2018

shoamano83 Apr 26, 2018

shoamano83 commented Apr 26, 2018

shoamano83 commented Jul 5, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

joeljfischer Jul 24, 2018

shoamano83 Jul 27, 2018

shoamano83 commented Aug 2, 2018

joeljfischer Aug 2, 2018

shoamano83 Aug 7, 2018

joeljfischer Aug 2, 2018

shoamano83 Aug 7, 2018

TCP Transport Rewrite #946

TCP Transport Rewrite #946

Conversation

shoamano83 commented Apr 26, 2018

Risk

Testing Plan

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

CLA

Choose a reason for hiding this comment

shoamano83 commented Apr 26, 2018

shoamano83 commented Jul 5, 2018

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

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

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

shoamano83 commented Aug 2, 2018

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