Merge pull request #70 from Fitbit/subscription_deadlock

GATT service subscription failure dur to incorrect state and anothe NPE fix
Fitbit · Jul 27, 2022 · eb99c65 · eb99c65
2 parents 097c282 + 2db0d15
commit eb99c65
Show file tree

Hide file tree

Showing 26 changed files with 84 additions and 75 deletions.
diff --git a/src/debug/java/com/fitbit/bluetooth/fbgatt/tx/mocks/AddGattServerServiceMockTransaction.java b/src/debug/java/com/fitbit/bluetooth/fbgatt/tx/mocks/AddGattServerServiceMockTransaction.java
@@ -56,8 +56,8 @@ protected void transaction(GattTransactionCallback callback) {
                         .gattState(getGattServer().getGattState())
                         .resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
                 callCallbackWithTransactionResultAndRelease(callback, transactionResultBuilder.build());
-                getGattServer().setState(GattState.IDLE);
             }
+            getGattServer().resetState();
         }, REASONABLE_AMOUNT_OF_TIME_FOR_ADDING_SERVICE);
     }
 

diff --git a/src/debug/java/com/fitbit/bluetooth/fbgatt/tx/mocks/GattServerConnectMockTransaction.java b/src/debug/java/com/fitbit/bluetooth/fbgatt/tx/mocks/GattServerConnectMockTransaction.java
@@ -46,14 +46,13 @@ public void transaction(GattTransactionCallback callback) {
                 getGattServer().setState(GattState.FAILURE_CONNECTING);
                 builder.resultStatus(TransactionResult.TransactionResultStatus.FAILURE)
                         .gattState(getGattServer().getGattState());
-                callCallbackWithTransactionResultAndRelease(callback, builder.build());
             } else {
                 getGattServer().setState(GattState.CONNECTED);
                 builder.resultStatus(TransactionResult.TransactionResultStatus.SUCCESS)
                         .gattState(getGattServer().getGattState());
-                callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                getGattServer().setState(GattState.IDLE);
             }
+            callCallbackWithTransactionResultAndRelease(callback, builder.build());
+            getGattServer().resetState();
         }, REASONABLE_AMOUNT_OF_TIME_FOR_CONNECT);
     }
 }
diff --git a/...a/com/fitbit/bluetooth/fbgatt/tx/mocks/NotifyGattServerCharacteristicMockTransaction.java b/...a/com/fitbit/bluetooth/fbgatt/tx/mocks/NotifyGattServerCharacteristicMockTransaction.java
@@ -62,7 +62,7 @@ protected void transaction(GattTransactionCallback callback) {
                     }
                     getGattServer().setState(GattState.NOTIFY_CHARACTERISTIC_FAILURE);
                     builder.resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
-                    getGattServer().setState(GattState.IDLE);
+                    getGattServer().resetState();
                     Strategy strategy = strategyProvider.
                             getStrategyForPhoneAndGattConnection(null, null,
                                     Situation.TRACKER_WENT_AWAY_DURING_GATT_OPERATION);

diff --git a/...ebug/java/com/fitbit/bluetooth/fbgatt/tx/mocks/SendGattServerResponseMockTransaction.java b/...ebug/java/com/fitbit/bluetooth/fbgatt/tx/mocks/SendGattServerResponseMockTransaction.java
@@ -43,21 +43,16 @@ protected void transaction(GattTransactionCallback callback) {
             if (shouldFail) {
                 getGattServer().setState(GattState.SEND_SERVER_RESPONSE_FAILURE);
                 builder.resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
-                builder.gattState(getGattServer().getGattState()).
-                        data(value).
-                        requestId(requestId).
-                        offset(offset);
-                callCallbackWithTransactionResultAndRelease(callback, builder.build());
             } else {
                 getGattServer().setState(GattState.SEND_SERVER_RESPONSE_SUCCESS);
                 builder.resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
-                builder.gattState(getGattServer().getGattState()).
-                        data(value).
-                        requestId(requestId).
-                        offset(offset);
-                callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                getGattServer().setState(GattState.IDLE);
             }
+            builder.gattState(getGattServer().getGattState()).
+                    data(value).
+                    requestId(requestId).
+                    offset(offset);
+            callCallbackWithTransactionResultAndRelease(callback, builder.build());
+            getGattServer().resetState();
         }, REASONABLE_TIME_FOR_SEND_RESPONSE);
     }
 }
diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/FitbitGatt.java b/src/main/java/com/fitbit/bluetooth/fbgatt/FitbitGatt.java
@@ -1470,10 +1470,10 @@ Runnable tryAndStartGattServer(Context context, OpenGattServerCallback callback,
                         if (serverConnection != null) {
                             // We have a new server instance we need to replace it in the GattServerConnection
                             serverConnection.close();
-                            serverConnection.setState(GattState.IDLE);
+                            serverConnection.resetState();
                         }
                         setGattServerConnection(new GattServerConnection(gattServer, context.getMainLooper()));
-                        serverConnection.setState(GattState.IDLE);
+                        serverConnection.resetState();
                         callback.onGattServerStatus(true);
                         isGattServerStarting.set(false);
                         Timber.tag("FitbitGattServer").v("Gatt server successfully opened");
@@ -1663,6 +1663,7 @@ public void stopBackgroundScanWithRegularIntent(Context context, @Nullable Inten
         }
     }
 
+    @Nullable
     public GattServerConnection getServer() {
         return serverConnection;
     }

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/GattConnection.java b/src/main/java/com/fitbit/bluetooth/fbgatt/GattConnection.java
@@ -53,12 +53,14 @@ public class GattConnection implements Closeable {
     private @NonNull Handler mainHandler;
     private final FitbitGatt fitbitGatt = FitbitGatt.getInstance();
 
+    private static GattState DEFAULT_STATE = GattState.DISCONNECTED;
+
     public GattConnection(FitbitBluetoothDevice device, Looper mainLooper) {
         this.device = device;
         this.guard = new GattStateTransitionValidator<GattClientTransaction>();
         this.mockServices = new ArrayList<>(1);
         this.clientQueue = new TransactionQueueController(this);
-        this.state = GattState.DISCONNECTED;
+        this.state = DEFAULT_STATE;
         this.disconnectedTTL = new AtomicLong(FitbitGatt.MAX_TTL);
         this.mainHandler = new Handler(mainLooper);
     }
@@ -216,8 +218,11 @@ synchronized GattStateTransitionValidator.GuardState checkTransaction(GattClient
         return guard.checkTransaction(getGattState(), tx);
     }
 
-    void resetStates() {
-        this.setState(GattState.DISCONNECTED);
+    /**
+     * Reset to default connection state
+     */
+    public void resetState() {
+        this.setState(DEFAULT_STATE);
     }
 
     @VisibleForTesting

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/GattServerCallback.java b/src/main/java/com/fitbit/bluetooth/fbgatt/GattServerCallback.java
@@ -93,6 +93,8 @@ public void onConnectionStateChange(BluetoothDevice device, int status, int newS
         if (fitbitGatt.isSlowLoggingEnabled()) {
             Timber.v("[%s] onConnectionStateChange: Gatt Response Status %s", getDeviceMacFromDevice(device), GattStatus.getStatusForCode(status));
             Timber.d("[%s][Threading] Originally called on thread : %s", getDeviceMacFromDevice(device), Thread.currentThread().getName());
+        } else {
+            Timber.v("onConnectionStateChange: Gatt Response Status %s", GattStatus.getStatusForCode(status));
         }
         ArrayList<GattServerListener> copy = new ArrayList<>(listeners.size());
         copy.addAll(listeners);

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/GattServerConnection.java b/src/main/java/com/fitbit/bluetooth/fbgatt/GattServerConnection.java
@@ -14,14 +14,14 @@
 import android.os.Build;
 import android.os.Handler;
 import android.os.Looper;
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 import java.io.Closeable;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
-import androidx.annotation.NonNull;
-import androidx.annotation.Nullable;
-import androidx.annotation.VisibleForTesting;
 import timber.log.Timber;
 
 /**
@@ -42,11 +42,13 @@ public class GattServerConnection implements Closeable {
     private Handler mainHandler;
     private boolean mockMode;
 
+    private static GattState DEFAULT_STATE = GattState.IDLE;
+
     protected GattServerConnection(@Nullable BluetoothGattServer server, Looper looper) {
         this.server = server;
         this.serverQueue = new TransactionQueueController();
         this.guard = new GattStateTransitionValidator<>();
-        this.state = GattState.IDLE;
+        this.state = DEFAULT_STATE;
         this.mainHandler = new Handler(looper);
     }
 
@@ -87,9 +89,11 @@ public synchronized void setState(GattState state) {
         this.state = state;
     }
 
-    @SuppressWarnings("unused") // API Method
-    void resetStates(){
-        this.setState(GattState.DISCONNECTED);
+    /**
+     * Reset to default connection state
+     */
+    public void resetState(){
+        this.setState(DEFAULT_STATE);
     }
 
     @VisibleForTesting

diff --git a/...main/java/com/fitbit/bluetooth/fbgatt/strategies/BluetoothOffClearGattServerStrategy.java b/...main/java/com/fitbit/bluetooth/fbgatt/strategies/BluetoothOffClearGattServerStrategy.java
@@ -82,7 +82,7 @@ public void applyStrategy() {
             } catch (NullPointerException e) {
                 Timber.w(e, "There was an internal stack NPE, the Android BluetoothService probably crashed or already shut down");
             }
-            serverConn.setState(GattState.IDLE);
+            serverConn.resetState();
         });
     }
 }
diff --git a/.../fitbit/bluetooth/fbgatt/strategies/HandleTrackerVanishingUnderGattOperationStrategy.java b/.../fitbit/bluetooth/fbgatt/strategies/HandleTrackerVanishingUnderGattOperationStrategy.java
@@ -8,18 +8,19 @@
 
 package com.fitbit.bluetooth.fbgatt.strategies;
 
+import android.bluetooth.BluetoothGatt;
+import androidx.annotation.Nullable;
 import com.fitbit.bluetooth.fbgatt.AndroidDevice;
 import com.fitbit.bluetooth.fbgatt.FitbitGatt;
 import com.fitbit.bluetooth.fbgatt.GattConnection;
+import com.fitbit.bluetooth.fbgatt.GattServerConnection;
 import com.fitbit.bluetooth.fbgatt.GattState;
-import android.bluetooth.BluetoothGatt;
 import java.util.concurrent.TimeUnit;
-import androidx.annotation.Nullable;
 import timber.log.Timber;
 
 /**
  * In the case that we have a tracker that has disconnected, in the middle of a write / read /
- * server response, we should mark that connection as disconnected and force the timeout wait with
+ * server response, we should reset that connection and force the timeout wait with
  * a 133 unknown ( connection interface unknown ) gatt status.
  */
 
@@ -49,9 +50,10 @@ public void applyStrategy() {
         Timber.v("Waiting %dms for the client_if to dump", WAIT_TIME_FOR_DISCONNECTION);
         if (connection != null) {
             connection.getMainHandler().postDelayed(() -> {
-                connection.setState(GattState.DISCONNECTED);
-                if (FitbitGatt.getInstance().getServer() != null) {
-                    FitbitGatt.getInstance().getServer().setState(GattState.DISCONNECTED);
+                connection.resetState();
+                GattServerConnection server = FitbitGatt.getInstance().getServer();
+                if (server != null) {
+                    server.resetState();
                 }
                 Timber.v("Strategy is done, gatt can be used again");
             }, WAIT_TIME_FOR_DISCONNECTION);

diff --git a/...m/fitbit/bluetooth/fbgatt/tx/AddGattServerServiceCharacteristicDescriptorTransaction.java b/...m/fitbit/bluetooth/fbgatt/tx/AddGattServerServiceCharacteristicDescriptorTransaction.java
@@ -112,7 +112,7 @@ private void respondWithError(String message, GattTransactionCallback callback)
                 .descriptorUuid(descriptor.getUuid())
                 .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
         callCallbackWithTransactionResultAndRelease(callback, builder.build());
-        getGattServer().setState(GattState.IDLE);
+        getGattServer().resetState();
     }
 
     /**

diff --git a/...in/java/com/fitbit/bluetooth/fbgatt/tx/AddGattServerServiceCharacteristicTransaction.java b/...in/java/com/fitbit/bluetooth/fbgatt/tx/AddGattServerServiceCharacteristicTransaction.java
@@ -70,7 +70,7 @@ protected void transaction(GattTransactionCallback callback) {
                             .characteristicUuid(characteristic.getUuid())
                             .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
                     callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                    getGattServer().setState(GattState.IDLE);
+                    getGattServer().resetState();
                     return;
                 }
                 addCriticalDescriptorsIfNecessary(characteristic);
@@ -84,7 +84,7 @@ protected void transaction(GattTransactionCallback callback) {
                             .characteristicUuid(characteristic.getUuid())
                             .resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
                     callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                    getGattServer().setState(GattState.IDLE);
+                    getGattServer().resetState();
                 } else {
                     respondWithError("Couldn't add the characteristic to the local gatt service", callback);
                 }
@@ -124,7 +124,7 @@ private void respondWithError(String message, GattTransactionCallback callback)
         builder.gattState(getGattServer().getGattState())
                 .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
         callCallbackWithTransactionResultAndRelease(callback, builder.build());
-        getGattServer().setState(GattState.IDLE);
+        getGattServer().resetState();
     }
 
     /**

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/tx/AddGattServerServiceTransaction.java b/src/main/java/com/fitbit/bluetooth/fbgatt/tx/AddGattServerServiceTransaction.java
@@ -71,7 +71,7 @@ protected void transaction(GattTransactionCallback callback) {
                         .serviceUuid(service.getUuid())
                         .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
                 callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                getGattServer().setState(GattState.IDLE);
+                getGattServer().resetState();
                 return;
             }
             boolean success = false;
@@ -92,7 +92,7 @@ protected void transaction(GattTransactionCallback callback) {
                     .serviceUuid(service.getUuid())
                     .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
                 callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                getGattServer().setState(GattState.IDLE);
+                getGattServer().resetState();
             }
         }
     }
@@ -107,17 +107,15 @@ public void onServerServiceAdded(int status, BluetoothGattService service) {
             builder.gattState(getGattServer().getGattState())
                     .serviceUuid(service.getUuid())
                     .resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
-            callCallbackWithTransactionResultAndRelease(callback, builder.build());
-            getGattServer().setState(GattState.IDLE);
         } else {
             getGattServer().setState(GattState.ADD_SERVICE_FAILURE);
             Timber.e("The gatt service could not be added: %s", service.getUuid());
             builder.gattState(getGattServer().getGattState())
                     .serviceUuid(service.getUuid())
                     .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
-            callCallbackWithTransactionResultAndRelease(callback, builder.build());
-            getGattServer().setState(GattState.IDLE);
         }
+        callCallbackWithTransactionResultAndRelease(callback, builder.build());
+        getGattServer().resetState();
     }
 
     /**
@@ -127,7 +125,8 @@ public void onServerServiceAdded(int status, BluetoothGattService service) {
      */
 
     private boolean doesGattServerServiceAlreadyExist(BluetoothGattService service) {
-        return FitbitGatt.getInstance().getServer().getServer().getService(service.getUuid()) != null;
+        GattServerConnection connection = FitbitGatt.getInstance().getServer();
+        return connection!= null && connection.getServer().getService(service.getUuid()) != null;
     }
 
     @Override

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/tx/ClearServerServicesTransaction.java b/src/main/java/com/fitbit/bluetooth/fbgatt/tx/ClearServerServicesTransaction.java
@@ -41,7 +41,7 @@ protected void transaction(GattTransactionCallback callback) {
                 .resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
         mainThreadHandler.post(() -> {
             callCallbackWithTransactionResultAndRelease(callback, builder.build());
-            getGattServer().setState(GattState.IDLE);
+            getGattServer().resetState();
         });
     }
 

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/tx/CloseGattServerTransaction.java b/src/main/java/com/fitbit/bluetooth/fbgatt/tx/CloseGattServerTransaction.java
@@ -53,7 +53,7 @@ protected void transaction(GattTransactionCallback callback) {
                 .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
             mainThreadHandler.post(() -> {
                 callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                getGattServer().setState(GattState.IDLE);
+                getGattServer().resetState();
             });
         } else {
             BluetoothUtils bluetoothUtils = new BluetoothUtils();
@@ -67,7 +67,7 @@ protected void transaction(GattTransactionCallback callback) {
                     .resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
                 mainThreadHandler.post(() -> {
                     callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                    getGattServer().setState(GattState.IDLE);
+                    getGattServer().resetState();
                 });
             } catch (NullPointerException ex) {
                 Timber.w("As the client close can sometimes NPE internally, it is reasonable to assume that a similar thing occurred for the server");
@@ -76,7 +76,7 @@ protected void transaction(GattTransactionCallback callback) {
                     .resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
                 mainThreadHandler.post(() -> {
                     callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                    getGattServer().setState(GattState.IDLE);
+                    getGattServer().resetState();
                 });
             }
         }

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/tx/GattServerConnectTransaction.java b/src/main/java/com/fitbit/bluetooth/fbgatt/tx/GattServerConnectTransaction.java
@@ -65,14 +65,16 @@ public void onServerConnectionStateChange(BluetoothDevice device, int status, in
                 builder.resultStatus(TransactionResult.TransactionResultStatus.FAILURE)
                         .gattState(getGattServer().getGattState());
                 callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                getGattServer().setState(GattState.IDLE);
+                // reset state after communicating failure
+                getGattServer().resetState();
             }
         } else {
             getGattServer().setState(GattState.FAILURE_CONNECTING);
             builder.resultStatus(TransactionResult.TransactionResultStatus.FAILURE)
                     .gattState(getGattServer().getGattState());
             callCallbackWithTransactionResultAndRelease(callback, builder.build());
-            getGattServer().setState(GattState.DISCONNECTED);
+            // reset state after communicating failure
+            getGattServer().resetState();
         }
     }
 

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/tx/GattServerDisconnectTransaction.java b/src/main/java/com/fitbit/bluetooth/fbgatt/tx/GattServerDisconnectTransaction.java
@@ -63,6 +63,7 @@ public void onServerConnectionStateChange(BluetoothDevice device, int status, in
                 builder.gattState(getGattServer().getGattState())
                         .resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
                 callCallbackWithTransactionResultAndRelease(callback, builder.build());
+                getGattServer().resetState();
             } else if (newState == BluetoothProfile.STATE_CONNECTED) {
                 getGattServer().setState(GattState.CONNECTED);
                 builder.gattState(getGattServer().getGattState())

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/tx/GetGattServerServicesTransaction.java b/src/main/java/com/fitbit/bluetooth/fbgatt/tx/GetGattServerServicesTransaction.java
@@ -51,7 +51,7 @@ protected void transaction(GattTransactionCallback callback) {
                 .gattState(getGattServer().getGattState());
         mainThreadHandler.post(() -> {
             callCallbackWithTransactionResultAndRelease(callback, builder.build());
-            getGattServer().setState(GattState.IDLE);
+            getGattServer().resetState();
         });
     }
 

diff --git a/src/main/java/com/fitbit/bluetooth/fbgatt/tx/NotifyGattServerCharacteristicTransaction.java b/src/main/java/com/fitbit/bluetooth/fbgatt/tx/NotifyGattServerCharacteristicTransaction.java
@@ -71,7 +71,7 @@ protected void transaction(GattTransactionCallback callback) {
                     .gattState(getGattServer().getGattState());
             mainThreadHandler.post(() -> {
                 callCallbackWithTransactionResultAndRelease(callback, builder.build());
-                getGattServer().setState(GattState.IDLE);
+                getGattServer().resetState();
                 // we want to apply this strategy to every phone, so we will provide an empty target android
                 // device
                 Strategy strategy = strategyProvider.
@@ -96,13 +96,12 @@ public void onServerNotificationSent(BluetoothDevice device, int status) {
         if(status == BluetoothGatt.GATT_SUCCESS) {
             getGattServer().setState(GattState.NOTIFY_CHARACTERISTIC_SUCCESS);
             builder.resultStatus(TransactionResult.TransactionResultStatus.SUCCESS);
-            callCallbackWithTransactionResultAndRelease(callback, builder.build());
-            getGattServer().setState(GattState.IDLE);
         } else {
             getGattServer().setState(GattState.NOTIFY_CHARACTERISTIC_FAILURE);
             builder.resultStatus(TransactionResult.TransactionResultStatus.FAILURE);
-            callCallbackWithTransactionResultAndRelease(callback, builder.build());
         }
+        callCallbackWithTransactionResultAndRelease(callback, builder.build());
+        getGattServer().resetState();
     }
 
     @Override