From 7cf11476b956145f2dea7252eacdf02c7b8f1a7b Mon Sep 17 00:00:00 2001 From: sakal Date: Fri, 20 May 2016 02:41:02 -0700 Subject: [PATCH] Replace LooperExecutor with built-in class in Android AppRTC Demo LooperExecutor is only truly needed in WebSocketChannelClient because of WebSocketClient from autobanh requiring thread to have a looper. So LooperExecutor was left there but replaced everywhere else with built-in singleThreadExecutor/singleThreadScheduledExecutor. Motivation behind this change is that built-in class behaves better under testing environment and doesn't require hacky RobolectricLooperExecutor. Review-Url: https://codereview.webrtc.org/1992213002 Cr-Commit-Position: refs/heads/master@{#12823} --- .../src/org/appspot/apprtc/CallActivity.java | 16 +-- .../src/org/appspot/apprtc/CpuMonitor.java | 38 +++--- .../org/appspot/apprtc/DirectRTCClient.java | 13 +- .../appspot/apprtc/PeerConnectionClient.java | 11 +- .../org/appspot/apprtc/TCPChannelClient.java | 25 ++-- .../appspot/apprtc/util/LooperExecutor.java | 3 +- .../appspot/apprtc/DirectRTCClientTest.java | 7 +- .../appspot/apprtc/TCPChannelClientTest.java | 36 ++++-- .../util/RobolectricLooperExecutor.java | 118 ------------------ .../apprtc/test/PeerConnectionClientTest.java | 11 +- 10 files changed, 79 insertions(+), 199 deletions(-) delete mode 100644 webrtc/examples/androidjunit/src/org/appspot/apprtc/util/RobolectricLooperExecutor.java diff --git a/webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java b/webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java index 9109383f45..7447d059bf 100644 --- a/webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java +++ b/webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java @@ -10,11 +10,6 @@ package org.appspot.apprtc; -import org.appspot.apprtc.AppRTCClient.RoomConnectionParameters; -import org.appspot.apprtc.AppRTCClient.SignalingParameters; -import org.appspot.apprtc.PeerConnectionClient.PeerConnectionParameters; -import org.appspot.apprtc.util.LooperExecutor; - import android.app.Activity; import android.app.AlertDialog; import android.app.FragmentTransaction; @@ -30,12 +25,16 @@ import android.view.Window; import android.view.WindowManager.LayoutParams; import android.widget.Toast; +import org.appspot.apprtc.AppRTCClient.RoomConnectionParameters; +import org.appspot.apprtc.AppRTCClient.SignalingParameters; +import org.appspot.apprtc.PeerConnectionClient.PeerConnectionParameters; +import org.appspot.apprtc.util.LooperExecutor; import org.webrtc.EglBase; import org.webrtc.IceCandidate; import org.webrtc.PeerConnectionFactory; +import org.webrtc.RendererCommon.ScalingType; import org.webrtc.SessionDescription; import org.webrtc.StatsReport; -import org.webrtc.RendererCommon.ScalingType; import org.webrtc.SurfaceViewRenderer; /** @@ -245,7 +244,7 @@ public class CallActivity extends Activity appRtcClient = new WebSocketRTCClient(this, new LooperExecutor()); } else { Log.i(TAG, "Using DirectRTCClient because room name looks like an IP."); - appRtcClient = new DirectRTCClient(this, new LooperExecutor()); + appRtcClient = new DirectRTCClient(this); } // Create connection parameters. roomConnectionParameters = new RoomConnectionParameters( @@ -314,9 +313,6 @@ public class CallActivity extends Activity } activityRunning = false; rootEglBase.release(); - if (cpuMonitor != null) { - cpuMonitor.release(); - } super.onDestroy(); } diff --git a/webrtc/examples/androidapp/src/org/appspot/apprtc/CpuMonitor.java b/webrtc/examples/androidapp/src/org/appspot/apprtc/CpuMonitor.java index 9f604fdbe8..33cf7f7af4 100644 --- a/webrtc/examples/androidapp/src/org/appspot/apprtc/CpuMonitor.java +++ b/webrtc/examples/androidapp/src/org/appspot/apprtc/CpuMonitor.java @@ -23,8 +23,9 @@ import java.io.FileReader; import java.io.IOException; import java.util.Arrays; import java.util.Scanner; - -import org.appspot.apprtc.util.LooperExecutor; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; /** * Simple CPU monitor. The caller creates a CpuMonitor object which can then @@ -85,7 +86,7 @@ class CpuMonitor { // CPU frequency in percentage from maximum. private final MovingAverage frequencyScale; - private LooperExecutor executor; + private ScheduledExecutorService executor; private long lastStatLogTimeMs; private long[] cpuFreqMax; private int cpusPresent; @@ -159,33 +160,21 @@ class CpuMonitor { frequencyScale = new MovingAverage(MOVING_AVERAGE_SAMPLES); lastStatLogTimeMs = SystemClock.elapsedRealtime(); - executor = new LooperExecutor(); - executor.requestStart(); scheduleCpuUtilizationTask(); } - public void release() { - if (executor != null) { - Log.d(TAG, "release"); - executor.cancelScheduledTasks(); - executor.requestStop(); - executor = null; - } - } - public void pause() { if (executor != null) { Log.d(TAG, "pause"); - executor.cancelScheduledTasks(); + executor.shutdownNow(); + executor = null; } } public void resume() { - if (executor != null) { - Log.d(TAG, "resume"); - resetStat(); - scheduleCpuUtilizationTask(); - } + Log.d(TAG, "resume"); + resetStat(); + scheduleCpuUtilizationTask(); } public synchronized void reset() { @@ -209,13 +198,18 @@ class CpuMonitor { } private void scheduleCpuUtilizationTask() { - executor.cancelScheduledTasks(); + if (executor != null) { + executor.shutdownNow(); + executor = null; + } + + executor = Executors.newSingleThreadScheduledExecutor(); executor.scheduleAtFixedRate(new Runnable() { @Override public void run() { cpuUtilizationTask(); } - }, CPU_STAT_SAMPLE_PERIOD_MS); + }, 0, CPU_STAT_SAMPLE_PERIOD_MS, TimeUnit.MILLISECONDS); } private void cpuUtilizationTask() { diff --git a/webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java b/webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java index 3de0dc03b3..4b6c87719e 100644 --- a/webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java +++ b/webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java @@ -12,7 +12,6 @@ package org.appspot.apprtc; import android.util.Log; -import org.appspot.apprtc.util.LooperExecutor; import org.json.JSONArray; import org.json.JSONException; import org.json.JSONObject; @@ -21,6 +20,8 @@ import org.webrtc.PeerConnection; import org.webrtc.SessionDescription; import java.util.LinkedList; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -52,7 +53,7 @@ public class DirectRTCClient implements AppRTCClient, TCPChannelClient.TCPChanne + "(:(\\d+))?" ); - private final LooperExecutor executor; + private final ExecutorService executor; private final SignalingEvents events; private TCPChannelClient tcpClient; private RoomConnectionParameters connectionParameters; @@ -64,11 +65,10 @@ public class DirectRTCClient implements AppRTCClient, TCPChannelClient.TCPChanne // All alterations of the room state should be done from inside the looper thread. private ConnectionState roomState; - public DirectRTCClient(SignalingEvents events, LooperExecutor looperExecutor) { + public DirectRTCClient(SignalingEvents events) { this.events = events; - executor = looperExecutor; - executor.requestStart(); + executor = Executors.newSingleThreadExecutor(); roomState = ConnectionState.NEW; } @@ -148,6 +148,7 @@ public class DirectRTCClient implements AppRTCClient, TCPChannelClient.TCPChanne tcpClient.disconnect(); tcpClient = null; } + executor.shutdown(); } @Override @@ -295,13 +296,11 @@ public class DirectRTCClient implements AppRTCClient, TCPChannelClient.TCPChanne @Override public void onTCPError(String description) { reportError("TCP connection error: " + description); - executor.requestStop(); } @Override public void onTCPClose() { events.onChannelClose(); - executor.requestStop(); } // -------------------------------------------------------------------- diff --git a/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java b/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java index 8f30451183..bd4a3746ae 100644 --- a/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java +++ b/webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java @@ -16,7 +16,6 @@ import android.os.Environment; import android.util.Log; import org.appspot.apprtc.AppRTCClient.SignalingParameters; -import org.appspot.apprtc.util.LooperExecutor; import org.webrtc.AudioTrack; import org.webrtc.CameraEnumerationAndroid; import org.webrtc.DataChannel; @@ -46,6 +45,9 @@ import java.util.EnumSet; import java.util.LinkedList; import java.util.Timer; import java.util.TimerTask; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -88,7 +90,7 @@ public class PeerConnectionClient { private static final PeerConnectionClient instance = new PeerConnectionClient(); private final PCObserver pcObserver = new PCObserver(); private final SDPObserver sdpObserver = new SDPObserver(); - private final LooperExecutor executor; + private final ScheduledExecutorService executor; private PeerConnectionFactory factory; private PeerConnection peerConnection; @@ -219,11 +221,10 @@ public class PeerConnectionClient { } private PeerConnectionClient() { - executor = new LooperExecutor(); - // Looper thread is started once in private ctor and is used for all + // Executor thread is started once in private ctor and is used for all // peer connection API calls to ensure new peer connection factory is // created on the same thread as previously destroyed factory. - executor.requestStart(); + executor = Executors.newSingleThreadScheduledExecutor(); } public static PeerConnectionClient getInstance() { diff --git a/webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java b/webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java index 996483ec1d..7e09c9b3b3 100644 --- a/webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java +++ b/webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java @@ -12,7 +12,7 @@ package org.appspot.apprtc; import android.util.Log; -import org.appspot.apprtc.util.LooperExecutor; +import org.webrtc.ThreadUtils; import java.io.BufferedReader; import java.io.IOException; @@ -22,6 +22,7 @@ import java.net.InetAddress; import java.net.ServerSocket; import java.net.Socket; import java.net.UnknownHostException; +import java.util.concurrent.ExecutorService; /** * Replacement for WebSocketChannelClient for direct communication between two IP addresses. Handles @@ -34,7 +35,8 @@ import java.net.UnknownHostException; public class TCPChannelClient { private static final String TAG = "TCPChannelClient"; - private final LooperExecutor executor; + private final ExecutorService executor; + private final ThreadUtils.ThreadChecker executorThreadCheck; private final TCPChannelEvents eventListener; private TCPSocket socket; @@ -58,8 +60,10 @@ public class TCPChannelClient { * @param port Port to listen on or connect to. */ public TCPChannelClient( - LooperExecutor executor, TCPChannelEvents eventListener, String ip, int port) { + ExecutorService executor, TCPChannelEvents eventListener, String ip, int port) { this.executor = executor; + executorThreadCheck = new ThreadUtils.ThreadChecker(); + executorThreadCheck.detachThread(); this.eventListener = eventListener; InetAddress address; @@ -83,7 +87,7 @@ public class TCPChannelClient { * Disconnects the client if not already disconnected. This will fire the onTCPClose event. */ public void disconnect() { - checkIfCalledOnValidThread(); + executorThreadCheck.checkIsOnValidThread(); socket.disconnect(); } @@ -94,7 +98,7 @@ public class TCPChannelClient { * @param message Message to be sent. */ public void send(String message) { - checkIfCalledOnValidThread(); + executorThreadCheck.checkIsOnValidThread(); socket.send(message); } @@ -112,17 +116,6 @@ public class TCPChannelClient { }); } - /** - * Helper method for debugging purposes. - * Ensures that TCPChannelClient method is called on a looper thread. - */ - private void checkIfCalledOnValidThread() { - if (!executor.checkOnLooperThread()) { - throw new IllegalStateException( - "TCPChannelClient method is not called on valid thread"); - } - } - /** * Base class for server and client sockets. Contains a listening thread that will call diff --git a/webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java b/webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java index 731630405a..6e2b251133 100644 --- a/webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java +++ b/webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java @@ -19,7 +19,8 @@ import java.util.List; import java.util.concurrent.Executor; /** - * Looper based executor class. + * Looper based executor class. This is needed because WebSocketClient from autobanh requires the + * thread to have a looper. The class is used in WebSocketRTCClient/WebSocketChannelClient. */ public class LooperExecutor extends Thread implements Executor { private static final String TAG = "LooperExecutor"; diff --git a/webrtc/examples/androidjunit/src/org/appspot/apprtc/DirectRTCClientTest.java b/webrtc/examples/androidjunit/src/org/appspot/apprtc/DirectRTCClientTest.java index ee3b7c4656..26ad536adf 100644 --- a/webrtc/examples/androidjunit/src/org/appspot/apprtc/DirectRTCClientTest.java +++ b/webrtc/examples/androidjunit/src/org/appspot/apprtc/DirectRTCClientTest.java @@ -10,9 +10,6 @@ package org.appspot.apprtc; -import android.util.Log; - -import org.appspot.apprtc.util.RobolectricLooperExecutor; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -56,8 +53,8 @@ public class DirectRTCClientTest { clientEvents = mock(AppRTCClient.SignalingEvents.class); serverEvents = mock(AppRTCClient.SignalingEvents.class); - client = new DirectRTCClient(clientEvents, new RobolectricLooperExecutor()); - server = new DirectRTCClient(serverEvents, new RobolectricLooperExecutor()); + client = new DirectRTCClient(clientEvents); + server = new DirectRTCClient(serverEvents); } @Test diff --git a/webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java b/webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java index e0e29630d1..bdfcf76787 100644 --- a/webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java +++ b/webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java @@ -10,7 +10,7 @@ package org.appspot.apprtc; -import org.appspot.apprtc.util.RobolectricLooperExecutor; + import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -21,6 +21,11 @@ import org.robolectric.RobolectricTestRunner; import org.robolectric.annotation.Config; import org.robolectric.shadows.ShadowLog; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + import static org.junit.Assert.fail; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; @@ -38,13 +43,14 @@ public class TCPChannelClientTest { private static final int CONNECT_TIMEOUT = 100; private static final int SEND_TIMEOUT = 100; private static final int DISCONNECT_TIMEOUT = 100; + private static final int TERMINATION_TIMEOUT = 1000; private static final String TEST_MESSAGE_SERVER = "Hello, Server!"; private static final String TEST_MESSAGE_CLIENT = "Hello, Client!"; @Mock TCPChannelClient.TCPChannelEvents serverEvents; @Mock TCPChannelClient.TCPChannelEvents clientEvents; - private RobolectricLooperExecutor executor; + private ExecutorService executor; private TCPChannelClient server; private TCPChannelClient client; @@ -55,15 +61,14 @@ public class TCPChannelClientTest { MockitoAnnotations.initMocks(this); - executor = new RobolectricLooperExecutor(); - executor.requestStart(); + executor = Executors.newSingleThreadExecutor(); } @After public void tearDown() { verifyNoMoreEvents(); - executor.executeAndWait(new Runnable() { + executeAndWait(new Runnable() { @Override public void run() { client.disconnect(); @@ -72,9 +77,9 @@ public class TCPChannelClientTest { }); // Stop the executor thread - executor.requestStop(); + executor.shutdown(); try { - executor.join(); + executor.awaitTermination(TERMINATION_TIMEOUT, TimeUnit.MILLISECONDS); } catch (InterruptedException e) { fail(e.getMessage()); } @@ -112,7 +117,7 @@ public class TCPChannelClientTest { public void testSendData() { testConnectIPv4(); - executor.executeAndWait(new Runnable() { + executeAndWait(new Runnable() { @Override public void run() { client.send(TEST_MESSAGE_SERVER); @@ -127,7 +132,7 @@ public class TCPChannelClientTest { @Test public void testDisconnectServer() { testConnectIPv4(); - executor.executeAndWait(new Runnable() { + executeAndWait(new Runnable() { @Override public void run() { server.disconnect(); @@ -141,7 +146,7 @@ public class TCPChannelClientTest { @Test public void testDisconnectClient() { testConnectIPv4(); - executor.executeAndWait(new Runnable() { + executeAndWait(new Runnable() { @Override public void run() { client.disconnect(); @@ -183,4 +188,15 @@ public class TCPChannelClientTest { verifyNoMoreInteractions(serverEvents); verifyNoMoreInteractions(clientEvents); } + + /** + * Queues runnable to be run and waits for it to be executed by the executor thread + */ + public void executeAndWait(Runnable runnable) { + try { + executor.submit(runnable).get(); + } catch (Exception e) { + fail(e.getMessage()); + } + } } diff --git a/webrtc/examples/androidjunit/src/org/appspot/apprtc/util/RobolectricLooperExecutor.java b/webrtc/examples/androidjunit/src/org/appspot/apprtc/util/RobolectricLooperExecutor.java deleted file mode 100644 index 19d595d304..0000000000 --- a/webrtc/examples/androidjunit/src/org/appspot/apprtc/util/RobolectricLooperExecutor.java +++ /dev/null @@ -1,118 +0,0 @@ -/* - * Copyright 2016 The WebRTC Project Authors. All rights reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -package org.appspot.apprtc.util; - -import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.BlockingQueue; - -import static org.junit.Assert.fail; - -/** - * LooperExecutor that doesn't use Looper because its implementation in Robolectric is not suited - * for our needs. Also implements executeAndWait that can be used to wait until the runnable has - * been executed. - */ -public class RobolectricLooperExecutor extends LooperExecutor { - private volatile boolean running = false; - private static final int RUNNABLE_QUEUE_CAPACITY = 256; - private final BlockingQueue runnableQueue - = new ArrayBlockingQueue<>(RUNNABLE_QUEUE_CAPACITY); - private long threadId; - - /** - * Executes the runnable passed to the constructor and sets isDone flag afterwards. - */ - private static class ExecuteAndWaitRunnable implements Runnable { - public boolean isDone = false; - private final Runnable runnable; - - ExecuteAndWaitRunnable(Runnable runnable) { - this.runnable = runnable; - } - - @Override - public void run() { - runnable.run(); - - synchronized (this) { - isDone = true; - notifyAll(); - } - } - } - - @Override - public void run() { - threadId = Thread.currentThread().getId(); - - while (running) { - final Runnable runnable; - - try { - runnable = runnableQueue.take(); - } catch (InterruptedException e) { - if (running) { - fail(e.getMessage()); - } - return; - } - - runnable.run(); - } - } - - @Override - public synchronized void requestStart() { - if (running) { - return; - } - running = true; - start(); - } - - @Override - public synchronized void requestStop() { - running = false; - interrupt(); - } - - @Override - public synchronized void execute(Runnable runnable) { - try { - runnableQueue.put(runnable); - } catch (InterruptedException e) { - fail(e.getMessage()); - } - } - - /** - * Queues runnable to be run and waits for it to be executed by the executor thread - */ - public void executeAndWait(Runnable runnable) { - ExecuteAndWaitRunnable executeAndWaitRunnable = new ExecuteAndWaitRunnable(runnable); - execute(executeAndWaitRunnable); - - synchronized (executeAndWaitRunnable) { - while (!executeAndWaitRunnable.isDone) { - try { - executeAndWaitRunnable.wait(); - } catch (InterruptedException e) { - fail(e.getMessage()); - } - } - } - } - - @Override - public boolean checkOnLooperThread() { - return (Thread.currentThread().getId() == threadId); - } -} diff --git a/webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java b/webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java index ab37520ab0..bba182fa1e 100644 --- a/webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java +++ b/webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java @@ -13,13 +13,15 @@ package org.appspot.apprtc.test; import java.util.LinkedList; import java.util.List; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import org.appspot.apprtc.AppRTCClient.SignalingParameters; import org.appspot.apprtc.PeerConnectionClient; import org.appspot.apprtc.PeerConnectionClient.PeerConnectionEvents; import org.appspot.apprtc.PeerConnectionClient.PeerConnectionParameters; -import org.appspot.apprtc.util.LooperExecutor; import org.webrtc.EglBase; import org.webrtc.IceCandidate; import org.webrtc.MediaCodecVideoEncoder; @@ -66,7 +68,7 @@ public class PeerConnectionClientTest extends InstrumentationTestCase private EglBase eglBase; // These are protected by their respective event objects. - private LooperExecutor signalingExecutor; + private ExecutorService signalingExecutor; private boolean isClosed; private boolean isIceConnected; private SessionDescription localSdp; @@ -279,8 +281,7 @@ public class PeerConnectionClientTest extends InstrumentationTestCase @Override public void setUp() { - signalingExecutor = new LooperExecutor(); - signalingExecutor.requestStart(); + signalingExecutor = Executors.newSingleThreadExecutor(); if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN_MR1) { eglBase = EglBase.create(); } @@ -288,7 +289,7 @@ public class PeerConnectionClientTest extends InstrumentationTestCase @Override public void tearDown() { - signalingExecutor.requestStop(); + signalingExecutor.shutdown(); if (eglBase != null) { eglBase.release(); }