Merge pull request #1454 from smartdevicelink/feature/issue_1412

Fix potential race condition in SdlProtocolBase

1 files changed, 96 insertions, 74 deletions

diff --git a/base/src/main/java/com/smartdevicelink/protocol/SdlProtocolBase.java b/base/src/main/java/com/smartdevicelink/protocol/SdlProtocolBase.java
index 18eebbbbc..d54f2de3a 100644
--- a/base/src/main/java/com/smartdevicelink/protocol/SdlProtocolBase.java
+++ b/base/src/main/java/com/smartdevicelink/protocol/SdlProtocolBase.java
@@ -94,6 +94,8 @@ public class SdlProtocolBase {
     // Lock to ensure all frames are sent uninterrupted
     private final Object FRAME_LOCK = new Object();
 
+    private final Object TRANSPORT_MANAGER_LOCK = new Object();
+
     private final ISdlProtocol iSdlProtocol;
     private final Hashtable<Integer, SdlProtocol.MessageFrameAssembler> _assemblerForMessageID = new Hashtable<>();
     private final Hashtable<Byte, Object> _messageLocks = new Hashtable<>();
@@ -155,16 +157,20 @@ public class SdlProtocolBase {
     } // end-ctor
 
     void setTransportManager(TransportManagerBase transportManager){
-        this.transportManager = transportManager;
+        synchronized (TRANSPORT_MANAGER_LOCK) {
+            this.transportManager = transportManager;
+        }
     }
 
-    public void start(){
-        if(transportManager == null){
-            throw new IllegalStateException("Attempting to start without setting a transport manager.");
+    public void start() {
+        synchronized (TRANSPORT_MANAGER_LOCK) {
+            if (transportManager == null) {
+                throw new IllegalStateException("Attempting to start without setting a transport manager.");
+            }
+            transportManager.start();
         }
-        transportManager.start();
-
     }
+
     /**
      * Retrieves the max payload size for a packet to be sent to the module
      * @return the max transfer unit
@@ -185,14 +191,18 @@ public class SdlProtocolBase {
     }
 
     public void resetSession (){
-        if(transportManager == null){
-            throw new IllegalStateException("Attempting to reset session without setting a transport manager.");
+        synchronized (TRANSPORT_MANAGER_LOCK) {
+            if (transportManager == null) {
+                throw new IllegalStateException("Attempting to reset session without setting a transport manager.");
+            }
+            transportManager.resetSession();
         }
-        transportManager.resetSession();
     }
 
     public boolean isConnected(){
-        return transportManager != null && transportManager.isConnected(null,null);
+        synchronized (TRANSPORT_MANAGER_LOCK) {
+            return transportManager != null && transportManager.isConnected(null, null);
+        }
     }
 
     /**
@@ -449,12 +459,14 @@ public class SdlProtocolBase {
         if (supportedSecondaryTransports != null) {
             for (TransportType supportedSecondary : supportedSecondaryTransports) {
                 if(!onlyHighBandwidth || supportedSecondary == TransportType.USB || supportedSecondary == TransportType.TCP) {
-                    if (transportManager != null && transportManager.isConnected(supportedSecondary, null)) {
-                        //A supported secondary transport is already connected
-                        return true;
-                    } else if (secondaryTransportParams != null && secondaryTransportParams.containsKey(supportedSecondary)) {
-                        //A secondary transport is available to connect to
-                        return true;
+                    synchronized (TRANSPORT_MANAGER_LOCK) {
+                        if (transportManager != null && transportManager.isConnected(supportedSecondary, null)) {
+                            //A supported secondary transport is already connected
+                            return true;
+                        } else if (secondaryTransportParams != null && secondaryTransportParams.containsKey(supportedSecondary)) {
+                            //A secondary transport is available to connect to
+                            return true;
+                        }
                     }
                 }
             }
@@ -534,10 +546,11 @@ public class SdlProtocolBase {
     public void endSession(byte sessionID) {
         SdlPacket header = SdlPacketFactory.createEndSession(SessionType.RPC, sessionID, hashID, (byte)protocolVersion.getMajor(), hashID);
         handlePacketToSend(header);
-        if(transportManager != null) {
-            transportManager.close(sessionID);
+        synchronized (TRANSPORT_MANAGER_LOCK) {
+            if (transportManager != null) {
+                transportManager.close(sessionID);
+            }
         }
-
     } // end-method
 
     public void sendMessage(ProtocolMessage protocolMsg) {
@@ -781,26 +794,27 @@ public class SdlProtocolBase {
                     }
                 };
 
-                if (transportManager != null) {
-                    if (transportManager.isConnected(secondaryTransportType, null)) {
-                        //The transport is actually connected, however no service has been registered
-                        listenerList.add(secondaryListener);
-                        registerSecondaryTransport(sessionID, transportManager.getTransportRecord(secondaryTransportType, null));
-                    } else if (secondaryTransportParams != null && secondaryTransportParams.containsKey(secondaryTransportType)) {
-                        //No acceptable secondary transport is connected, so first one must be connected
-                        header.setTransportRecord(new TransportRecord(secondaryTransportType, ""));
-                        listenerList.add(secondaryListener);
-                        transportManager.requestSecondaryTransportConnection(sessionID, secondaryTransportParams.get(secondaryTransportType));
+                synchronized (TRANSPORT_MANAGER_LOCK) {
+                    if (transportManager != null) {
+                        if (transportManager.isConnected(secondaryTransportType, null)) {
+                            //The transport is actually connected, however no service has been registered
+                            listenerList.add(secondaryListener);
+                            registerSecondaryTransport(sessionID, transportManager.getTransportRecord(secondaryTransportType, null));
+                        } else if (secondaryTransportParams != null && secondaryTransportParams.containsKey(secondaryTransportType)) {
+                            //No acceptable secondary transport is connected, so first one must be connected
+                            header.setTransportRecord(new TransportRecord(secondaryTransportType, ""));
+                            listenerList.add(secondaryListener);
+                            transportManager.requestSecondaryTransportConnection(sessionID, secondaryTransportParams.get(secondaryTransportType));
+                        } else {
+                            DebugTool.logWarning(TAG, "No params to connect to secondary transport");
+                            //Unable to register or start a secondary connection. Use the callback in case
+                            //there is a chance to use the primary transport for this service.
+                            secondaryListener.onConnectionFailure();
+                        }
                     } else {
-                        DebugTool.logWarning(TAG, "No params to connect to secondary transport");
-                        //Unable to register or start a secondary connection. Use the callback in case
-                        //there is a chance to use the primary transport for this service.
-                        secondaryListener.onConnectionFailure();
+                        DebugTool.logError(TAG, "transportManager is null");
                     }
-                } else {
-                    DebugTool.logError(TAG, "transportManager is null");
                 }
-
             }
         }
     }
@@ -836,11 +850,11 @@ public class SdlProtocolBase {
      */
     protected void handlePacketToSend(SdlPacket packet) {
         synchronized(FRAME_LOCK) {
-
-            if(packet!=null && transportManager != null) {
-                transportManager.sendPacket(packet);
+            synchronized (TRANSPORT_MANAGER_LOCK) {
+                if (packet != null && transportManager != null) {
+                    transportManager.sendPacket(packet);
+                }
             }
-
         }
     }
 
@@ -1142,8 +1156,10 @@ public class SdlProtocolBase {
             TransportRecord transportRecord = getTransportForSession(SessionType.RPC);
             if(transportRecord == null && !requestedSession){ //There is currently no transport registered
                 requestedSession = true;
-                if (transportManager != null) {
-                    transportManager.requestNewSession(getPreferredTransport(requestedPrimaryTransports, connectedTransports));
+                synchronized (TRANSPORT_MANAGER_LOCK) {
+                    if (transportManager != null) {
+                        transportManager.requestNewSession(getPreferredTransport(requestedPrimaryTransports, connectedTransports));
+                    }
                 }
             }
             onTransportsConnectedUpdate(connectedTransports);
@@ -1156,8 +1172,10 @@ public class SdlProtocolBase {
         public void onTransportDisconnected(String info, TransportRecord disconnectedTransport, List<TransportRecord> connectedTransports) {
             if (disconnectedTransport == null) {
                 DebugTool.logInfo(TAG, "onTransportDisconnected");
-                if (transportManager != null) {
-                    transportManager.close(iSdlProtocol.getSessionId());
+                synchronized (TRANSPORT_MANAGER_LOCK) {
+                    if (transportManager != null) {
+                        transportManager.close(iSdlProtocol.getSessionId());
+                    }
                 }
                 iSdlProtocol.shutdown("No transports left connected");
                 return;
@@ -1185,45 +1203,49 @@ public class SdlProtocolBase {
                     for (TransportType transportType: requestedPrimaryTransports){
                         DebugTool.logInfo(TAG,  "Checking " + transportType.name());
 
-                        if(!disconnectedTransport.getType().equals(transportType)
-                                && transportManager != null
-                                && transportManager.isConnected(transportType,null)){
-
-                            //There is currently a supported primary transport
-
-                            //See if any high bandwidth transport is available currently
-                            boolean highBandwidthAvailable = transportManager.isHighBandwidthAvailable();
-
-                            if (requiresHighBandwidth) {
-                                if (!highBandwidthAvailable) {
-                                    if (TransportType.BLUETOOTH.equals(transportType)
-                                            && requestedSecondaryTransports != null
-                                            && supportedSecondaryTransports != null) {
-                                        for (TransportType secondaryTransport : requestedSecondaryTransports) {
-                                            DebugTool.logInfo(TAG, "Checking secondary " + secondaryTransport.name());
-                                            if (supportedSecondaryTransports.contains(secondaryTransport)) {
-                                                //Should only be USB or TCP
-                                                highBandwidthAvailable = true;
-                                                break;
+                        synchronized (TRANSPORT_MANAGER_LOCK) {
+                            if (!disconnectedTransport.getType().equals(transportType)
+                                    && transportManager != null
+                                    && transportManager.isConnected(transportType, null)) {
+
+                                //There is currently a supported primary transport
+
+                                //See if any high bandwidth transport is available currently
+                                boolean highBandwidthAvailable = transportManager.isHighBandwidthAvailable();
+
+                                if (requiresHighBandwidth) {
+                                    if (!highBandwidthAvailable) {
+                                        if (TransportType.BLUETOOTH.equals(transportType)
+                                                && requestedSecondaryTransports != null
+                                                && supportedSecondaryTransports != null) {
+                                            for (TransportType secondaryTransport : requestedSecondaryTransports) {
+                                                DebugTool.logInfo(TAG, "Checking secondary " + secondaryTransport.name());
+                                                if (supportedSecondaryTransports.contains(secondaryTransport)) {
+                                                    //Should only be USB or TCP
+                                                    highBandwidthAvailable = true;
+                                                    break;
+                                                }
                                             }
                                         }
-                                    }
-                                } // High bandwidth already available
-                            }
+                                    } // High bandwidth already available
+                                }
 
-                            if(!requiresHighBandwidth  || (requiresHighBandwidth && highBandwidthAvailable )) {
-                                primaryTransportAvailable = true;
-                                transportManager.updateTransportConfig(transportConfig);
-                                break;
+                                if (!requiresHighBandwidth || (requiresHighBandwidth && highBandwidthAvailable)) {
+                                    primaryTransportAvailable = true;
+                                    transportManager.updateTransportConfig(transportConfig);
+                                    break;
+                                }
                             }
                         }
                     }
                 }
                 connectedPrimaryTransport = null;
-                if (transportManager != null) {
-                    transportManager.close(iSdlProtocol.getSessionId());
+                synchronized (TRANSPORT_MANAGER_LOCK) {
+                    if (transportManager != null) {
+                        transportManager.close(iSdlProtocol.getSessionId());
+                    }
+                    transportManager = null;
                 }
-                transportManager = null;
                 requestedSession = false;
 
                 activeTransports.clear();