From ec49173e3a566b0b29f4571c2bf44afb0a2a080e Mon Sep 17 00:00:00 2001 From: Artur Dryomov Date: Fri, 13 Sep 2013 02:10:15 +0300 Subject: [PATCH] Fix deadlocks while connecting. Replace an events loop at the communication service with single-shot connection workers. This should help to avoid deadlock behaviour in situations when user quits the connection to one server and tries to connect to another one. The events loop required synchronization blocks which cause ANR in the situation described above. Change the server connection interface to contain a connecting stage in a separate method. This is more just-in-case measure for situations when the connection constructor shouldn't be blocking. Change-Id: I941a4b67d965f6b1f76bc9975818e82aea12bf00 --- .../activity/SlideShowActivity.java | 2 - .../BluetoothServerConnection.java | 45 ++-- .../communication/CommunicationService.java | 243 +++++++----------- .../communication/ServerConnection.java | 6 +- .../communication/TcpServerConnection.java | 48 ++-- .../fragment/ComputerConnectionFragment.java | 42 ++- 6 files changed, 181 insertions(+), 205 deletions(-) diff --git a/android/sdremote/src/org/libreoffice/impressremote/activity/SlideShowActivity.java b/android/sdremote/src/org/libreoffice/impressremote/activity/SlideShowActivity.java index 19b7dbbbf1d2..2d611340f8a7 100644 --- a/android/sdremote/src/org/libreoffice/impressremote/activity/SlideShowActivity.java +++ b/android/sdremote/src/org/libreoffice/impressremote/activity/SlideShowActivity.java @@ -502,8 +502,6 @@ public class SlideShowActivity extends SherlockFragmentActivity implements Servi stopTimer(); - // TODO: disconnect computer - unbindService(); } diff --git a/android/sdremote/src/org/libreoffice/impressremote/communication/BluetoothServerConnection.java b/android/sdremote/src/org/libreoffice/impressremote/communication/BluetoothServerConnection.java index fe5834d9a8a2..1056b7dfe116 100644 --- a/android/sdremote/src/org/libreoffice/impressremote/communication/BluetoothServerConnection.java +++ b/android/sdremote/src/org/libreoffice/impressremote/communication/BluetoothServerConnection.java @@ -14,10 +14,11 @@ import java.io.InputStream; import java.io.OutputStream; import java.util.UUID; -import android.bluetooth.BluetoothAdapter; import android.bluetooth.BluetoothDevice; import android.bluetooth.BluetoothSocket; +import org.libreoffice.impressremote.util.BluetoothOperator; + class BluetoothServerConnection implements ServerConnection { // Standard UUID for the Serial Port Profile. // https://www.bluetooth.org/en-us/specification/assigned-numbers-overview/service-discovery @@ -31,18 +32,31 @@ class BluetoothServerConnection implements ServerConnection { private BluetoothSocket buildServerConnection(Server aServer) { try { - BluetoothDevice aBluetoothServer = BluetoothAdapter - .getDefaultAdapter().getRemoteDevice(aServer.getAddress()); + BluetoothDevice aBluetoothServer = BluetoothOperator.getAdapter() + .getRemoteDevice(aServer.getAddress()); - BluetoothSocket aServerConnection = aBluetoothServer - .createRfcommSocketToServiceRecord( - UUID.fromString(STANDARD_SPP_UUID)); - - aServerConnection.connect(); - - return aServerConnection; + return aBluetoothServer.createRfcommSocketToServiceRecord( + UUID.fromString(STANDARD_SPP_UUID)); } catch (IOException e) { - throw new RuntimeException("Unable to connect to Bluetooth host."); + throw new RuntimeException("Unable to create server connection."); + } + } + + @Override + public void open() { + try { + mServerConnection.connect(); + } catch (IOException e) { + throw new RuntimeException("Unable to open server connection."); + } + } + + @Override + public void close() { + try { + mServerConnection.close(); + } catch (IOException e) { + throw new RuntimeException("Unable to close server connection."); } } @@ -63,15 +77,6 @@ class BluetoothServerConnection implements ServerConnection { throw new RuntimeException("Unable to open commands stream."); } } - - @Override - public void close() { - try { - mServerConnection.close(); - } catch (IOException e) { - throw new RuntimeException("Unable to close server connection."); - } - } } /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/android/sdremote/src/org/libreoffice/impressremote/communication/CommunicationService.java b/android/sdremote/src/org/libreoffice/impressremote/communication/CommunicationService.java index d7915c3a9d57..b4285f100cf3 100644 --- a/android/sdremote/src/org/libreoffice/impressremote/communication/CommunicationService.java +++ b/android/sdremote/src/org/libreoffice/impressremote/communication/CommunicationService.java @@ -20,43 +20,23 @@ import org.libreoffice.impressremote.util.BluetoothOperator; import org.libreoffice.impressremote.util.Intents; public class CommunicationService extends Service implements Runnable, MessagesListener, Timer.TimerListener { - public static enum State { - DISCONNECTED, SEARCHING, CONNECTING, CONNECTED - } - - /** - * Used to protect all writes to mState, mStateDesired, and mServerDesired. - */ - private final Object mConnectionVariableMutex = new Object(); - - private State mState; - private State mStateDesired; - - private Server mServerDesired; - private IBinder mBinder; private ServersManager mServersManager; - private ServerConnection mServerConnection; - - private MessagesReceiver mMessagesReceiver; - private CommandsTransmitter mCommandsTransmitter; - private BluetoothOperator.State mBluetoothState; private Timer mTimer; private SlideShow mSlideShow; - private Thread mThread; + private Server mServer; + private ServerConnection mServerConnection; + + private MessagesReceiver mMessagesReceiver; + private CommandsTransmitter mCommandsTransmitter; @Override public void onCreate() { - mState = State.DISCONNECTED; - mStateDesired = State.DISCONNECTED; - - mServerDesired = null; - mBinder = new CBinder(); mServersManager = new ServersManager(this); @@ -66,9 +46,6 @@ public class CommunicationService extends Service implements Runnable, MessagesL mTimer = new Timer(this); mSlideShow = new SlideShow(mTimer); - - mThread = new Thread(this); - mThread.start(); } public class CBinder extends Binder { @@ -90,92 +67,7 @@ public class CommunicationService extends Service implements Runnable, MessagesL return mBinder; } - @Override - public void run() { - synchronized (this) { - while (true) { - // Condition - try { - wait(); - } catch (InterruptedException e) { - // We have finished - return; - } - - // Work - synchronized (mConnectionVariableMutex) { - if ((mStateDesired == State.CONNECTED) && (mState == State.CONNECTED)) { - closeConnection(); - } - - if ((mStateDesired == State.DISCONNECTED) && (mState == State.CONNECTED)) { - closeConnection(); - } - - if (mStateDesired == State.CONNECTED) { - mState = State.CONNECTING; - - try { - openConnection(); - } - catch (RuntimeException e) { - connectionFailed(); - } - } - } - } - } - } - - private void closeConnection() { - mServerConnection.close(); - - mState = State.DISCONNECTED; - } - - private void openConnection() { - mServerConnection = buildServerConnection(); - - mMessagesReceiver = new MessagesReceiver(mServerConnection, this); - mCommandsTransmitter = new CommandsTransmitter(mServerConnection); - - if (PairingProvider.isPairingNecessary(mServerDesired)) { - pair(); - } - - mState = State.CONNECTED; - } - - private ServerConnection buildServerConnection() { - switch (mServerDesired.getProtocol()) { - case TCP: - return new TcpServerConnection(mServerDesired); - - case BLUETOOTH: - return new BluetoothServerConnection(mServerDesired); - - default: - throw new RuntimeException("Unknown desired protocol."); - } - } - - private void pair() { - String aPairingDeviceName = PairingProvider.getPairingDeviceName(this); - String aPairingPin = PairingProvider.getPairingPin(this, mServerDesired); - - mCommandsTransmitter.pair(aPairingDeviceName, aPairingPin); - } - - private void connectionFailed() { - mState = State.DISCONNECTED; - - Intent aIntent = Intents.buildConnectionFailedIntent(); - LocalBroadcastManager.getInstance(this).sendBroadcast(aIntent); - } - public void startServersSearch() { - mState = State.SEARCHING; - mServersManager.startServersSearch(); } @@ -183,43 +75,6 @@ public class CommunicationService extends Service implements Runnable, MessagesL mServersManager.stopServersSearch(); } - public List getServers() { - return mServersManager.getServers(); - } - - public void connectTo(Server aServer) { - synchronized (mConnectionVariableMutex) { - if (mState == State.SEARCHING) { - mServersManager.stopServersSearch(); - mState = State.DISCONNECTED; - } - mServerDesired = aServer; - mStateDesired = State.CONNECTED; - synchronized (this) { - notify(); - } - - } - // TODO: connect - } - - public void disconnect() { - synchronized (mConnectionVariableMutex) { - mStateDesired = State.DISCONNECTED; - synchronized (this) { - notify(); - } - } - } - - public CommandsTransmitter getTransmitter() { - return mCommandsTransmitter; - } - - public SlideShow getSlideShow() { - return mSlideShow; - } - public void addServer(String aAddress, String aName) { mServersManager.addTcpServer(aAddress, aName); } @@ -228,9 +83,87 @@ public class CommunicationService extends Service implements Runnable, MessagesL mServersManager.removeServer(aServer); } + public List getServers() { + return mServersManager.getServers(); + } + + public void connectServer(Server aServer) { + mServer = aServer; + + new Thread(this).start(); + } + + @Override + public void run() { + try { + disconnectServer(); + connectServer(); + } + catch (RuntimeException e) { + sendConnectionFailedMessage(); + } + } + + private void connectServer() { + mServerConnection = buildServerConnection(); + mServerConnection.open(); + + mMessagesReceiver = new MessagesReceiver(mServerConnection, this); + mCommandsTransmitter = new CommandsTransmitter(mServerConnection); + + if (PairingProvider.isPairingNecessary(mServer)) { + pair(); + } + } + + private ServerConnection buildServerConnection() { + switch (mServer.getProtocol()) { + case TCP: + return new TcpServerConnection(mServer); + + case BLUETOOTH: + return new BluetoothServerConnection(mServer); + + default: + throw new RuntimeException("Unknown desired protocol."); + } + } + + private void pair() { + String aPairingDeviceName = PairingProvider.getPairingDeviceName(this); + String aPairingPin = PairingProvider.getPairingPin(this, mServer); + + mCommandsTransmitter.pair(aPairingDeviceName, aPairingPin); + } + + private void sendConnectionFailedMessage() { + Intent aIntent = Intents.buildConnectionFailedIntent(); + LocalBroadcastManager.getInstance(this).sendBroadcast(aIntent); + } + + public void disconnectServer() { + if (!isServerConnectionAvailable()) { + return; + } + + mServerConnection.close(); + } + + private boolean isServerConnectionAvailable() { + return mServerConnection != null; + } + + public CommandsTransmitter getTransmitter() { + return mCommandsTransmitter; + } + + public SlideShow getSlideShow() { + return mSlideShow; + } + @Override public void onPinValidation() { - String aPin = PairingProvider.getPairingPin(this, mServerDesired); + String aPin = PairingProvider.getPairingPin(this, mServer); Intent aIntent = Intents.buildPairingValidationIntent(aPin); LocalBroadcastManager.getInstance(this).sendBroadcast(aIntent); @@ -294,11 +227,9 @@ public class CommunicationService extends Service implements Runnable, MessagesL @Override public void onDestroy() { stopServersSearch(); + disconnectServer(); restoreBluetoothState(); - - mThread.interrupt(); - mThread = null; } private void restoreBluetoothState() { @@ -310,6 +241,10 @@ public class CommunicationService extends Service implements Runnable, MessagesL return; } + disableBluetooth(); + } + + private void disableBluetooth() { BluetoothOperator.disable(); } } diff --git a/android/sdremote/src/org/libreoffice/impressremote/communication/ServerConnection.java b/android/sdremote/src/org/libreoffice/impressremote/communication/ServerConnection.java index e0bb49d40562..c9179d2ef209 100644 --- a/android/sdremote/src/org/libreoffice/impressremote/communication/ServerConnection.java +++ b/android/sdremote/src/org/libreoffice/impressremote/communication/ServerConnection.java @@ -12,11 +12,13 @@ import java.io.InputStream; import java.io.OutputStream; interface ServerConnection { + public void open(); + + public void close(); + public InputStream buildMessagesStream(); public OutputStream buildCommandsStream(); - - public void close(); } /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/android/sdremote/src/org/libreoffice/impressremote/communication/TcpServerConnection.java b/android/sdremote/src/org/libreoffice/impressremote/communication/TcpServerConnection.java index a37d353221b7..fa18079f3d86 100644 --- a/android/sdremote/src/org/libreoffice/impressremote/communication/TcpServerConnection.java +++ b/android/sdremote/src/org/libreoffice/impressremote/communication/TcpServerConnection.java @@ -11,26 +11,45 @@ package org.libreoffice.impressremote.communication; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.net.InetSocketAddress; import java.net.Socket; -import java.net.UnknownHostException; +import java.net.SocketAddress; class TcpServerConnection implements ServerConnection { + private final Server mServer; private final Socket mServerConnection; public TcpServerConnection(Server aServer) { - mServerConnection = buildServerConnection(aServer); + mServer = aServer; + mServerConnection = buildServerConnection(); } - private Socket buildServerConnection(Server aServer) { - try { - String aServerAddress = aServer.getAddress(); - int aServerPort = Protocol.Ports.CLIENT_CONNECTION; + private Socket buildServerConnection() { + return new Socket(); + } - return new Socket(aServerAddress, aServerPort); - } catch (UnknownHostException e) { - throw new RuntimeException("Unable to connect to unknown host."); + @Override + public void open() { + try { + mServerConnection.connect(buildServerAddress()); } catch (IOException e) { - throw new RuntimeException("Unable to connect to host."); + throw new RuntimeException("Unable to open server connection."); + } + } + + private SocketAddress buildServerAddress() { + String aServerAddress = mServer.getAddress(); + int aServerPort = Protocol.Ports.CLIENT_CONNECTION; + + return new InetSocketAddress(aServerAddress, aServerPort); + } + + @Override + public void close() { + try { + mServerConnection.close(); + } catch (IOException e) { + throw new RuntimeException("Unable to close server connection."); } } @@ -51,15 +70,6 @@ class TcpServerConnection implements ServerConnection { throw new RuntimeException("Unable to open commands stream."); } } - - @Override - public void close() { - try { - mServerConnection.close(); - } catch (IOException e) { - throw new RuntimeException("Unable to close server connection."); - } - } } /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/android/sdremote/src/org/libreoffice/impressremote/fragment/ComputerConnectionFragment.java b/android/sdremote/src/org/libreoffice/impressremote/fragment/ComputerConnectionFragment.java index f72944ab1a2a..ec0eeb5d1ff5 100644 --- a/android/sdremote/src/org/libreoffice/impressremote/fragment/ComputerConnectionFragment.java +++ b/android/sdremote/src/org/libreoffice/impressremote/fragment/ComputerConnectionFragment.java @@ -37,7 +37,11 @@ import org.libreoffice.impressremote.communication.Server; import org.libreoffice.impressremote.util.SavedStates; public class ComputerConnectionFragment extends SherlockFragment implements ServiceConnection { - private Server mComputer; + public static enum Result { + CONNECTED, NOT_CONNECTED + } + + private Result mResult = Result.NOT_CONNECTED; private CommunicationService mCommunicationService; private BroadcastReceiver mIntentsReceiver; @@ -62,8 +66,6 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv public void onCreate(Bundle aSavedInstance) { super.onCreate(aSavedInstance); - mComputer = getArguments().getParcelable(Fragments.Arguments.COMPUTER); - setUpActionBarMenu(); } @@ -136,10 +138,10 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv CommunicationService.CBinder aServiceBinder = (CommunicationService.CBinder) aBinder; mCommunicationService = aServiceBinder.getService(); - connectToComputer(); + connectComputer(); } - private void connectToComputer() { + private void connectComputer() { if (!isServiceBound()) { return; } @@ -148,7 +150,7 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv return; } - mCommunicationService.connectTo(mComputer); + mCommunicationService.connectServer(getComputer()); } private boolean isServiceBound() { @@ -163,6 +165,10 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv return (ProgressBar) getView().findViewById(R.id.progress_bar); } + private Server getComputer() { + return getArguments().getParcelable(Fragments.Arguments.COMPUTER); + } + @Override public void onServiceDisconnected(ComponentName aComponentName) { mCommunicationService = null; @@ -247,6 +253,8 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv } private void setUpPresentation() { + mResult = Result.CONNECTED; + Intent aIntent = Intents.buildSlideShowIntent(getActivity()); startActivity(aIntent); @@ -261,7 +269,7 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv } private String buildSecondaryErrorMessage() { - switch (mComputer.getProtocol()) { + switch (getComputer().getProtocol()) { case BLUETOOTH: return getString(R.string.message_impress_pairing_check); @@ -311,7 +319,7 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv switch (aMenuItem.getItemId()) { case R.id.menu_reconnect: showProgressBar(); - connectToComputer(); + connectComputer(); refreshActionBarMenu(); return true; @@ -374,9 +382,27 @@ public class ComputerConnectionFragment extends SherlockFragment implements Serv public void onDestroy() { super.onDestroy(); + disconnectComputer(); + unbindService(); } + private void disconnectComputer() { + if (!isServiceBound()) { + return; + } + + if (!isDisconnectRequired()) { + return; + } + + mCommunicationService.disconnectServer(); + } + + private boolean isDisconnectRequired() { + return mResult == Result.NOT_CONNECTED; + } + private void unbindService() { if (!isServiceBound()) { return;