From bd8c83db8f424a8cfbf94cd4915d7b99b034358c Mon Sep 17 00:00:00 2001 From: Maksymilian Osowski Date: Fri, 3 Sep 2010 12:32:12 +0100 Subject: [PATCH] Fixed the bug in forwarder that prevented ConnectionHandler threads from exiting. There was a deadlock when ConnectionHandler exited the loop in SocketPipeThread, and would call the onFinishedCallback, which called the synchronized method in Forwarder that would deadlock. Changing the Forwarder class solved the issue and made it more efficient. Change-Id: I947450a19573f2e88274b1ebc7b77d4df6afffa7 --- .../forwarder/ConnectionHandler.java | 18 ++- .../dumprendertree2/forwarder/Forwarder.java | 123 ++++++++---------- 2 files changed, 72 insertions(+), 69 deletions(-) diff --git a/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/ConnectionHandler.java b/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/ConnectionHandler.java index c356a103040d9..5e9f24ee7836d 100644 --- a/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/ConnectionHandler.java +++ b/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/ConnectionHandler.java @@ -56,6 +56,7 @@ public class ConnectionHandler { } } catch (IOException e) { Log.w(LOG_TAG, this.toString(), e); + finish(); return; } @@ -74,8 +75,17 @@ public class ConnectionHandler { } } - ConnectionHandler.this.stop(); - mOnFinishedCallback.onFinished(); + finish(); + } + + private void finish() { + synchronized (mThreadsRunning) { + mThreadsRunning--; + if (mThreadsRunning == 0) { + ConnectionHandler.this.stop(); + mOnFinishedCallback.onFinished(); + } + } } @Override @@ -84,6 +94,8 @@ public class ConnectionHandler { } } + private Integer mThreadsRunning; + private Socket mFromSocket, mToSocket; private SocketPipeThread mFromToPipe, mToFromPipe; @@ -101,6 +113,8 @@ public class ConnectionHandler { } public void start() { + /** We have 2 threads running, one for each pipe, that we start here. */ + mThreadsRunning = 2; mFromToPipe.start(); mToFromPipe.start(); } diff --git a/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/Forwarder.java b/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/Forwarder.java index 1b581fc727ab0..31cd8ea12738a 100644 --- a/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/Forwarder.java +++ b/tests/DumpRenderTree2/src/com/android/dumprendertree2/forwarder/Forwarder.java @@ -34,7 +34,6 @@ public class Forwarder extends Thread { private int mPort; private String mRemoteMachineIpAddress; - private Boolean mIsRunning = false; private ServerSocket mServerSocket; private Set mConnectionHandlers = new HashSet(); @@ -55,71 +54,70 @@ public class Forwarder extends Thread { return; } - mIsRunning = true; super.start(); } @Override public void run() { while (true) { - synchronized (this) { - if (!mIsRunning) { - return; - } - - /** These sockets will be closed when Forwarder.stop() is called */ - Socket localSocket; - Socket remoteSocket; - try { - localSocket = mServerSocket.accept(); - remoteSocket = AdbUtils.getSocketToRemoteMachine(mRemoteMachineIpAddress, - mPort); - } catch (IOException e) { - /** This most likely means that mServerSocket is already closed */ - Log.w(LOG_TAG, "mPort=" + mPort, e); - return; - } - - if (remoteSocket == null) { - try { - localSocket.close(); - } catch (IOException e) { - Log.e(LOG_TAG, "mPort=" + mPort, e); - } - - Log.e(LOG_TAG, "run(): mPort= " + mPort + " Failed to start forwarding from " + - localSocket); - continue; - } - - final ConnectionHandler connectionHandler = - new ConnectionHandler(localSocket, remoteSocket); - - /** - * We have to close the sockets after the ConnectionHandler finishes, so we - * don't get "Too may open files" exception. We also remove the ConnectionHandler - * from the collection to avoid memory issues. - * */ - ConnectionHandler.OnFinishedCallback callback = - new ConnectionHandler.OnFinishedCallback() { - @Override - public void onFinished() { - removeConncetionHandler(connectionHandler); - } - }; - connectionHandler.registerOnConnectionHandlerFinishedCallback(callback); - - mConnectionHandlers.add(connectionHandler); - connectionHandler.start(); + /** These sockets will be closed when Forwarder.stop() is called */ + Socket localSocket; + Socket remoteSocket; + try { + localSocket = mServerSocket.accept(); + remoteSocket = AdbUtils.getSocketToRemoteMachine(mRemoteMachineIpAddress, + mPort); + } catch (IOException e) { + /** This most likely means that mServerSocket is already closed */ + Log.w(LOG_TAG, "mPort=" + mPort, e); + break; } - } - } - private synchronized void removeConncetionHandler(ConnectionHandler connectionHandler) { - if (mConnectionHandlers.remove(connectionHandler)) { - Log.d(LOG_TAG, "removeConnectionHandler(): removed"); - } else { - Log.d(LOG_TAG, "removeConnectionHandler(): not in the collection"); + if (remoteSocket == null) { + try { + localSocket.close(); + } catch (IOException e) { + Log.e(LOG_TAG, "mPort=" + mPort, e); + } + + Log.e(LOG_TAG, "run(): mPort= " + mPort + " Failed to start forwarding from " + + localSocket); + continue; + } + + final ConnectionHandler connectionHandler = + new ConnectionHandler(localSocket, remoteSocket); + + /** + * We have to close the sockets after the ConnectionHandler finishes, so we + * don't get "Too may open files" exception. We also remove the ConnectionHandler + * from the collection to avoid memory issues. + * */ + ConnectionHandler.OnFinishedCallback callback = + new ConnectionHandler.OnFinishedCallback() { + @Override + public void onFinished() { + synchronized (this) { + if (mConnectionHandlers.remove(connectionHandler)) { + Log.d(LOG_TAG, "removeConnectionHandler(): removed"); + } else { + assert false : "removeConnectionHandler(): not in the collection"; + } + } + } + }; + connectionHandler.registerOnConnectionHandlerFinishedCallback(callback); + + synchronized (this) { + mConnectionHandlers.add(connectionHandler); + } + connectionHandler.start(); + } + + synchronized (this) { + for (ConnectionHandler connectionHandler : mConnectionHandlers) { + connectionHandler.stop(); + } } } @@ -129,14 +127,5 @@ public class Forwarder extends Thread { } catch (IOException e) { Log.e(LOG_TAG, "mPort=" + mPort, e); } - - synchronized (this) { - mIsRunning = false; - } - - for (ConnectionHandler connectionHandler : mConnectionHandlers) { - connectionHandler.stop(); - } - mConnectionHandlers.clear(); } } \ No newline at end of file