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.

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

TCP Transport Rewrite #946

Merged
merged 9 commits into from Aug 14, 2018

Conversation

Projects
None yet
2 participants
@shoamano83
Contributor

shoamano83 commented Apr 26, 2018

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

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

This comment has been minimized.

@shoamano83

shoamano83 Apr 26, 2018

Contributor

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

This comment has been minimized.

Contributor

shoamano83 commented Apr 26, 2018

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!

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

@joeljfischer joeljfischer added this to the 6.1.0 milestone May 24, 2018

shoamano83 added some commits Apr 23, 2018

Add notification for TCP connection errors
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 shoamano83 force-pushed the XevoInc:feature/rewrite_tcp_transport branch from 5cf9017 to 6bf797f Jul 5, 2018

@shoamano83

This comment has been minimized.

Contributor

shoamano83 commented Jul 5, 2018

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

@@ -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;

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

Please rename sdl_transport_unknownError

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

Done in commit 72e4cb3.

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

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

SDLTransportErrorUnknown

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

Done in commit 72e4cb3.

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

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

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

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

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

[self disconnect];
}
#pragma mark - SDLAbstractTransport methods

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

There is no more SDLAbstractTransport

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

Renamed with commit 72e4cb3.

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

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

self.ioThread.name = TCPIOThreadName;

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

Fixed in commit 72e4cb3.

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

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

Should this be an error log?

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

Done in commit 4aa7820.

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

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

Please use braces as noted in the comment above

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

Done in commit 72e4cb3.

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

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

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?

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

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:.

}
}
- (void)sdl_doNothing {
}

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

Just one line - (void)sdl_doNothing {}

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

Done in commit 72e4cb3.

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

This comment has been minimized.

@joeljfischer

joeljfischer Jul 24, 2018

Member

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

This comment has been minimized.

@shoamano83

shoamano83 Jul 27, 2018

Contributor

OK, done in commit 713f409.

shoamano83 added some commits Jul 27, 2018

Reflect review comments
- Rename an error enum
- Update / remove comments, pragmas and logs
- Resolve styling issues
Reflect review comments
- Change receiveBufferSize to private
- Remove `default` case from switch statement
- Simplify some if statements
- Update logs
Reflect review comments
- Use NSAssert to check a flag that should be always false
Merge branch 'develop' into feature/rewrite_tcp_transport
Conflicts:
	SmartDeviceLink-iOS.xcodeproj/project.pbxproj
Merge branch 'develop' into feature/rewrite_tcp_transport
Conflicts:
	SmartDeviceLink-iOS.xcodeproj/project.pbxproj
@shoamano83

This comment has been minimized.

Contributor

shoamano83 commented Aug 2, 2018

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.

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)

This comment has been minimized.

@joeljfischer

joeljfischer Aug 3, 2018

Member

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

This comment has been minimized.

@shoamano83

shoamano83 Aug 7, 2018

Contributor

OK, added in commit c1883ff.

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 comment has been minimized.

@joeljfischer

joeljfischer Aug 3, 2018

Member

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

This comment has been minimized.

@shoamano83

shoamano83 Aug 7, 2018

Contributor

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.

Reflect review comment
- Add assertions in I/O callback methods

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joeljfischer joeljfischer referenced this pull request Aug 14, 2018

Merged

v6.1.0 Release #999

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment