Core disconnect crash

[joeljfischer]

Bug Report

If Core is disconnected while an app is connected, that app will crash.

Reproduction Steps

1. Connect to SDL Core using an app running the SDL SDK using TCP
2. Close SDL Core while the app is still connected

Expected Behavior

The app to close its connection gracefully

Observed Behavior

The app crashes

OS & Version Information

• iOS Version: 9.3.0
• SDL iOS Version: 4.1.0
• Testing Against: SDL Core 4.0.0

[asm09fsu]

@joeljfischer is this existent on iAP2, or only TCP/IP?

[joeljfischer]

I think it's only TCP/IP, but I'm not sure.

[asm09fsu]

To solve this issue, we can actually check the TCPCallback c function in SDLTCPTransport. Looking at the code below, we add an if statement to check the length of the data on the transport session. If the length is 0, we can assume that the transport has been cancelled by the server.

} else if (kCFSocketDataCallBack == type) {
        SDLTCPTransport *transport = (__bridge SDLTCPTransport *)info;

        if (CFDataGetLength((CFDataRef)data) > 0) {
            NSMutableString *byteStr = [NSMutableString stringWithCapacity:((int)CFDataGetLength((CFDataRef)data) * 2)];
            for (int i = 0; i < (int)CFDataGetLength((CFDataRef)data); i++) {
                [byteStr appendFormat:@"%02X", ((Byte *)(UInt8 *)CFDataGetBytePtr((CFDataRef)data))[i]];
            }

            [SDLDebugTool logInfo:[NSString stringWithFormat:@"Read %d bytes: %@", (int)CFDataGetLength((CFDataRef)data), byteStr] withType:SDLDebugType_Transport_TCP toOutput:SDLDebugOutput_DeviceConsole];

            [transport.delegate onDataReceived:[NSData dataWithBytes:(UInt8 *)CFDataGetBytePtr((CFDataRef)data) length:(int)CFDataGetLength((CFDataRef)data)]];
        } else {
            [SDLDebugTool logInfo:@"Could not connect to Core. Disconnecting…" withType:SDLDebugType_Protocol toOutput:SDLDebugOutput_File | SDLDebugOutput_DeviceConsole];
            [transport.delegate onTransportDisconnected];
        }
    } else {

The only issue we run in to, however, is that it doesn't seem there is a way for us to properly dispose of the proxy and transport. If we issue this delegate function call, based on the logs it appears that the transport gets re-initialized, followed by the proxy.

2016-04-05 20:52:57.320 SmartDeviceLink-Example[83708:9565055] [1] Could not connect to Core. Disconnecting…
2016-04-05 20:52:57.320 SmartDeviceLink-Example[83708:9565055] [1] SDLTCPTransport Init
2016-04-05 20:52:57.321 SmartDeviceLink-Example[83708:9565055] [1] Init
2016-04-05 20:52:57.321 SmartDeviceLink-Example[83708:9565055] [1] Server Not Ready, Connection Failed
2016-04-05 20:52:57.321 SmartDeviceLink-Example[83708:9565055] [1] SDLProxy initWithTransport
2016-04-05 20:52:57.322 SmartDeviceLink-Example[83708:9565055] [1] SDLProxy Dealloc
2016-04-05 20:52:57.322 SmartDeviceLink-Example[83708:9565055] [1] SDLProtocol Dealloc

We should try to see if there is a way for us to properly dispose of the transports and proxy.

[joeljfischer]

It appears that there currently is not a way to do this well. We may have to wait for v5.0.0 to rearchitecture portions of the proxy to handle dealloc'ing in the proper order.

[justinjdickow]

@asm09fsu I think this fix is valid. It seems like the reason why you see SDLTCPTransport Init after calling OnProxyDisconnect is that the Example App implements OnProxyClosed as

- (void)onProxyClosed {
    [self resetProxyWithTransportType:self.currentTransportType];
}

If you agree I'll issue a hot fix - I am getting annoyed by this problem every day.

[joeljfischer]

@justinjdickow That's the correct code for onProxyClosed right now, what are you suggesting in its place?

[justinjdickow]

Nothing, it's fine the way it is. It's the exact same behavior as trying to start the proxy the way you are in startProxyWithTransportType. It just fails with that log if it can't start/restart.

[asm09fsu]

The issue just comes back to the fact that we don't handle the fact that we'lol get a data call back when the connection is severed from the host. If we look here we get some explanation

[justinjdickow]

Right, which I why I said your solution is valid, checking the data length is another indication that the connected was severed by the host. Since we don't handle it, it manages to propagate itself up to trying to determine the packet type and crashes in an exception we throw (for some reason). Instead, if we call onTransportDisconnected from that point (where data isn't > 0), the disconnect is propagated to the developer. In our case, we're just trying to connect again and fail. We could implement some reachability check but I at least prefer that silent failure over the crash that is currently happening.

[joeljfischer]

The problem Alex saw is that there's a race condition preventing the automatic reconnection attempt. There would be a failure, then never an attempt to reconnect without killing and restarting the app.

[joeljfischer]

I tested the fix myself on a branch, and I think it's fine to pull in for now. It's definitely not okay for production, that would need a full retry strategy, but for debug, it's useful.