PBAP OBEX session uses Handler/HandlerThread now.

PBAP was using Java threads which was making synchronization error prone
(as revealed by just connecting/disconnecting pbap alongwith pulling
contacts). This change uses Handler to post messages and cleans up the
internal state of the OBEX client in general.

Also, make the BluetoothSocket close non-blocking. Before this change
the close() call was waiting on BluetoothSocket#open() to finish which
has a deadlock.

Change-Id: I0d8b3d08e19f6f34f0e115d08227b029c6c6751c

2 files changed, 159 insertions, 133 deletions

diff --git a/src/android/bluetooth/client/pbap/BluetoothPbapObexSession.java b/src/android/bluetooth/client/pbap/BluetoothPbapObexSession.java
index f558cc4..48b240c 100644
--- a/src/android/bluetooth/client/pbap/BluetoothPbapObexSession.java
+++ b/src/android/bluetooth/client/pbap/BluetoothPbapObexSession.java
@@ -17,9 +17,13 @@
 package android.bluetooth.client.pbap;
 
 import android.os.Handler;
+import android.os.HandlerThread;
+import android.os.Looper;
+import android.os.Message;
 import android.util.Log;
 
 import java.io.IOException;
+import java.lang.ref.WeakReference;
 
 import javax.obex.ClientSession;
 import javax.obex.HeaderSet;
@@ -27,6 +31,7 @@ import javax.obex.ObexTransport;
 import javax.obex.ResponseCodes;
 
 final class BluetoothPbapObexSession {
+    private static final boolean DBG = true;
     private static final String TAG = "BluetoothPbapObexSession";
 
     private static final byte[] PBAP_TARGET = new byte[] {
@@ -42,191 +47,207 @@ final class BluetoothPbapObexSession {
     final static int OBEX_SESSION_AUTHENTICATION_REQUEST = 105;
     final static int OBEX_SESSION_AUTHENTICATION_TIMEOUT = 106;
 
+    final static int MSG_CONNECT = 0;
+    final static int MSG_REQUEST = 1;
+
+    final static int CONNECTED = 0;
+    final static int CONNECTING = 1;
+    final static int DISCONNECTED = 2;
+
     private Handler mSessionHandler;
     private final ObexTransport mTransport;
-    private ObexClientThread mObexClientThread;
+    // private ObexClientThread mObexClientThread;
     private BluetoothPbapObexAuthenticator mAuth = null;
+    private HandlerThread mThread;
+    private Handler mHandler;
+    private ClientSession mClientSession;
+
+    private int mState = DISCONNECTED;
 
     public BluetoothPbapObexSession(ObexTransport transport) {
         mTransport = transport;
     }
 
-    public void start(Handler handler) {
+    public synchronized boolean start(Handler handler) {
         Log.d(TAG, "start");
+
+        if (mState == CONNECTED || mState == CONNECTING) {
+            return false;
+        }
+        mState = CONNECTING;
         mSessionHandler = handler;
 
         mAuth = new BluetoothPbapObexAuthenticator(mSessionHandler);
 
-        mObexClientThread = new ObexClientThread();
-        mObexClientThread.start();
+        // Start the thread to process requests (see {@link schedule()}.
+        mThread = new HandlerThread("BluetoothPbapObexSessionThread");
+        mThread.start();
+        mHandler = new ObexClientHandler(mThread.getLooper(), this);
+
+        // Make connect call non blocking.
+        boolean status = mHandler.sendMessage(mHandler.obtainMessage(MSG_CONNECT));
+        if (!status) {
+            mState = DISCONNECTED;
+            return false;
+        } else {
+            return true;
+        }
     }
 
     public void stop() {
-        Log.d(TAG, "stop");
-
-        if (mObexClientThread != null) {
-            try {
-                mObexClientThread.interrupt();
-                mObexClientThread.join();
-                mObexClientThread = null;
-            } catch (InterruptedException e) {
-            }
+        if (DBG) {
+            Log.d(TAG, "stop");
         }
+
+        // This will essentially stop the handler and ignore any inflight requests.
+        mThread.quit();
+
+        // We clean up the state here.
+        disconnect(false /* no callback */);
     }
 
     public void abort() {
-        Log.d(TAG, "abort");
-
-        if (mObexClientThread != null && mObexClientThread.mRequest != null) {
-            /*
-             * since abort may block until complete GET is processed inside OBEX
-             * session, let's run it in separate thread so it won't block UI
-             */
-            (new Thread() {
-                @Override
-                public void run() {
-                    mObexClientThread.mRequest.abort();
-                }
-            }).run();
-        }
+        stop();
     }
 
     public boolean schedule(BluetoothPbapRequest request) {
-        Log.d(TAG, "schedule: " + request.getClass().getSimpleName());
-
-        if (mObexClientThread == null) {
-            Log.e(TAG, "OBEX session not started");
-            return false;
+        if (DBG) {
+            Log.d(TAG, "schedule() called with: " + request);
         }
 
-        return mObexClientThread.schedule(request);
-    }
-
-    public boolean setAuthReply(String key) {
-        Log.d(TAG, "setAuthReply key=" + key);
-
-        if (mAuth == null) {
+        boolean status = mHandler.sendMessage(mHandler.obtainMessage(MSG_REQUEST, request));
+        if (!status) {
+            Log.e(TAG, "Adding messages failed, obex must be disconnected.");
             return false;
         }
-
-        mAuth.setReply(key);
-
         return true;
     }
 
-    private class ObexClientThread extends Thread {
-
-        private static final String TAG = "ObexClientThread";
-
-        private ClientSession mClientSession;
-        private BluetoothPbapRequest mRequest;
-
-        private volatile boolean mRunning = true;
+    public int isConnected() {
+        return mState;
+    }
 
-        public ObexClientThread() {
+    private void connect() {
+       if (DBG) {
+          Log.d(TAG, "connect()");
+       }
+
+       boolean success = true;
+       try {
+          mClientSession = new ClientSession(mTransport);
+          mClientSession.setAuthenticator(mAuth);
+       } catch (IOException e) {
+          Log.d(TAG, "connect() exception: " + e);
+          success = false;
+       }
+
+       HeaderSet hs = new HeaderSet();
+       hs.setHeader(HeaderSet.TARGET, PBAP_TARGET);
+       try {
+          hs = mClientSession.connect(hs);
+
+          if (hs.getResponseCode() != ResponseCodes.OBEX_HTTP_OK) {
+              disconnect(true  /* callback */);
+              success = false;
+          }
+       } catch (IOException e) {
+          success = false;
+       }
+
+       synchronized (this) {
+           if (success) {
+              mSessionHandler.obtainMessage(OBEX_SESSION_CONNECTED).sendToTarget();
+              mState = CONNECTED;
+           } else {
+              mSessionHandler.obtainMessage(OBEX_SESSION_DISCONNECTED).sendToTarget();
+              mState = DISCONNECTED;
+           }
+       }
+    }
 
-            mClientSession = null;
-            mRequest = null;
+    private synchronized void disconnect(boolean callback) {
+        if (DBG) {
+            Log.d(TAG, "disconnect()");
         }
 
-        @Override
-        public void run() {
-            super.run();
-
-            if (!connect()) {
-                mSessionHandler.obtainMessage(OBEX_SESSION_FAILED).sendToTarget();
-                return;
-            }
+        if (mState != DISCONNECTED) {
+            return;
+        }
 
-            mSessionHandler.obtainMessage(OBEX_SESSION_CONNECTED).sendToTarget();
-
-            while (mRunning) {
-                synchronized (this) {
-                    try {
-                        if (mRequest == null) {
-                            this.wait();
-                        }
-                    } catch (InterruptedException e) {
-                        mRunning = false;
-                        break;
-                    }
-                }
-
-                if (mRunning && mRequest != null) {
-                    try {
-                        mRequest.execute(mClientSession);
-                    } catch (IOException e) {
-                        // this will "disconnect" for cleanup
-                        mRunning = false;
-                    }
-
-                    if (mRequest.isSuccess()) {
-                        mSessionHandler.obtainMessage(OBEX_SESSION_REQUEST_COMPLETED, mRequest)
-                                .sendToTarget();
-                    } else {
-                        mSessionHandler.obtainMessage(OBEX_SESSION_REQUEST_FAILED, mRequest)
-                                .sendToTarget();
-                    }
-                }
-
-                mRequest = null;
+        if (mClientSession != null) {
+            try {
+                mClientSession.disconnect(null);
+                mClientSession.close();
+            } catch (IOException e) {
             }
+        }
 
-            disconnect();
-
+        if (callback) {
             mSessionHandler.obtainMessage(OBEX_SESSION_DISCONNECTED).sendToTarget();
         }
 
-        public synchronized boolean schedule(BluetoothPbapRequest request) {
-            Log.d(TAG, "schedule: " + request.getClass().getSimpleName());
-
-            if (mRequest != null) {
-                return false;
-            }
+        mState = DISCONNECTED;
+    }
 
-            mRequest = request;
-            notify();
+    private void executeRequest(BluetoothPbapRequest req) {
+        try {
+            req.execute(mClientSession);
+        } catch (IOException e) {
+            Log.e(TAG, "Error during executeRequest " + e);
+            disconnect(true);
+        }
 
-            return true;
+        if (req.isSuccess()) {
+            mSessionHandler.obtainMessage(OBEX_SESSION_REQUEST_COMPLETED, req).sendToTarget();
+        } else {
+            mSessionHandler.obtainMessage(OBEX_SESSION_REQUEST_FAILED, req).sendToTarget();
         }
+    }
 
-        private boolean connect() {
-            Log.d(TAG, "connect");
+    public boolean setAuthReply(String key) {
+        Log.d(TAG, "setAuthReply key=" + key);
 
-            try {
-                mClientSession = new ClientSession(mTransport);
-                mClientSession.setAuthenticator(mAuth);
-            } catch (IOException e) {
-                return false;
-            }
+        if (mAuth == null) {
+            return false;
+        }
 
-            HeaderSet hs = new HeaderSet();
-            hs.setHeader(HeaderSet.TARGET, PBAP_TARGET);
+        mAuth.setReply(key);
 
-            try {
-                hs = mClientSession.connect(hs);
+        return true;
+    }
 
-                if (hs.getResponseCode() != ResponseCodes.OBEX_HTTP_OK) {
-                    disconnect();
-                    return false;
-                }
-            } catch (IOException e) {
-                return false;
-            }
+    private static class ObexClientHandler extends Handler {
+        WeakReference<BluetoothPbapObexSession> mInst;
 
-            return true;
+        ObexClientHandler(Looper looper, BluetoothPbapObexSession inst) {
+            super(looper);
+            mInst = new WeakReference<BluetoothPbapObexSession>(inst);
         }
 
-        private void disconnect() {
-            Log.d(TAG, "disconnect");
+        @Override
+        public void handleMessage(Message msg) {
+            BluetoothPbapObexSession inst = mInst.get();
+            if (inst == null) {
+                Log.e(TAG, "The instance class is no longer around; terminating.");
+                return;
+            }
+
+            if (inst.isConnected() != CONNECTED && msg.what != MSG_CONNECT) {
+                Log.w(TAG, "Cannot execute " + msg + " when not CONNECTED.");
+                return;
+            }
 
-            if (mClientSession != null) {
-                try {
-                    mClientSession.disconnect(null);
-                    mClientSession.close();
-                } catch (IOException e) {
-                }
+            switch (msg.what) {
+                case MSG_CONNECT:
+                    inst.connect();
+                    break;
+                case MSG_REQUEST:
+                    inst.executeRequest((BluetoothPbapRequest) msg.obj);
+                    break;
+                default:
+                    Log.e(TAG, "Unknwown message type: " + msg.what);
             }
         }
     }
 }
+
diff --git a/src/android/bluetooth/client/pbap/BluetoothPbapSession.java b/src/android/bluetooth/client/pbap/BluetoothPbapSession.java
index 70e0ac8..faafd0b 100644
--- a/src/android/bluetooth/client/pbap/BluetoothPbapSession.java
+++ b/src/android/bluetooth/client/pbap/BluetoothPbapSession.java
@@ -254,6 +254,10 @@ class BluetoothPbapSession implements Callback {
 
         if (mConnectThread != null) {
             try {
+                // Force close the socket in case the thread is stuck doing the connect()
+                // call.
+                mConnectThread.closeSocket();
+                // TODO: Add timed join if closeSocket does not clean up the state.
                 mConnectThread.join();
             } catch (InterruptedException e) {
             }
@@ -317,7 +321,8 @@ class BluetoothPbapSession implements Callback {
 
         }
 
-        private void closeSocket() {
+        // This method may be called from outside the thread if the connect() call above is stuck.
+        public void closeSocket() {
             try {
                 if (mSocket != null) {
                     mSocket.close();