From 588f0b83e15df0a8ef3930c4a02f4fe4087ab147 Mon Sep 17 00:00:00 2001 From: Felix Oghina Date: Fri, 9 Sep 2016 04:17:19 -0700 Subject: [PATCH] Actually close packager websocket connection when destroying instance Summary: Ugh. We never actually closed the websocket connection. Furthermore, upon calling `closeQuietly()`, `onClose()` is called, which does `reconnect()`. Beautiful. This results in `ReactInstanceManager` leaking after calling `destroy()` and nullifying all references to it. To fix this I made sure `closeQuietly()` actually closes the connection for good, **and** made sure we actually call it when destroying an instance. Reviewed By: AaaChiuuu Differential Revision: D3835023 fbshipit-source-id: 31811805dd97b725ea5887cffed9bed49addda83 --- .../react/devsupport/DevServerHelper.java | 22 +++++++++-- .../devsupport/DevSupportManagerImpl.java | 39 ++++++++++--------- .../devsupport/JSPackagerWebSocketClient.java | 30 +++++++++++--- 3 files changed, 64 insertions(+), 27 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java index 707b93303..38ee1bca2 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java @@ -79,7 +79,7 @@ public class DevServerHelper { } public interface PackagerCommandListener { - void onReload(); + void onPackagerReloadCommand(); } public interface PackagerStatusCallback { @@ -88,15 +88,15 @@ public class DevServerHelper { private final DevInternalSettings mSettings; private final OkHttpClient mClient; - private final JSPackagerWebSocketClient mPackagerConnection; private final Handler mRestartOnChangePollingHandler; private boolean mOnChangePollingEnabled; + private @Nullable JSPackagerWebSocketClient mPackagerConnection; private @Nullable OkHttpClient mOnChangePollingClient; private @Nullable OnServerContentChangeListener mOnServerContentChangeListener; private @Nullable Call mDownloadBundleFromURLCall; - public DevServerHelper(DevInternalSettings settings, final PackagerCommandListener commandListener) { + public DevServerHelper(DevInternalSettings settings) { mSettings = settings; mClient = new OkHttpClient.Builder() .connectTimeout(HTTP_CONNECT_TIMEOUT_MS, TimeUnit.MILLISECONDS) @@ -105,18 +105,32 @@ public class DevServerHelper { .build(); mRestartOnChangePollingHandler = new Handler(); + } + + public void openPackagerConnection(final PackagerCommandListener commandListener) { + if (mPackagerConnection != null) { + FLog.w(ReactConstants.TAG, "Packager connection already open, nooping."); + return; + } mPackagerConnection = new JSPackagerWebSocketClient(getPackagerConnectionURL(), new JSPackagerWebSocketClient.JSPackagerCallback() { @Override public void onMessage(String target, String action) { if (commandListener != null && "bridge".equals(target) && "reload".equals(action)) { - commandListener.onReload(); + commandListener.onPackagerReloadCommand(); } } }); mPackagerConnection.connect(); } + public void closePackagerConnection() { + if (mPackagerConnection != null) { + mPackagerConnection.closeQuietly(); + mPackagerConnection = null; + } + } + /** Intent action for reloading the JS */ public static String getReloadAppAction(Context context) { return context.getPackageName() + RELOAD_APP_ACTION_SUFFIX; diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java index 68eeec8bd..f126b9516 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java @@ -48,6 +48,7 @@ import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.common.ReactConstants; import com.facebook.react.common.ShakeDetector; import com.facebook.react.common.futures.SimpleSettableFuture; +import com.facebook.react.devsupport.DevServerHelper.PackagerCommandListener; import com.facebook.react.devsupport.StackTraceHelper.StackFrame; import com.facebook.react.modules.debug.DeveloperSettings; @@ -82,7 +83,7 @@ import okhttp3.RequestBody; * {@code } * {@code } */ -public class DevSupportManagerImpl implements DevSupportManager { +public class DevSupportManagerImpl implements DevSupportManager, PackagerCommandListener { private static final int JAVA_ERROR_COOKIE = -1; private static final int JSEXCEPTION_ERROR_COOKIE = -1; @@ -120,7 +121,6 @@ public class DevSupportManagerImpl implements DevSupportManager { private int mLastErrorCookie = 0; private @Nullable ErrorType mLastErrorType; - private static class JscProfileTask extends AsyncTask { private static final MediaType JSON = MediaType.parse("application/json; charset=utf-8"); @@ -178,19 +178,7 @@ public class DevSupportManagerImpl implements DevSupportManager { mApplicationContext = applicationContext; mJSAppBundleName = packagerPathForJSBundleName; mDevSettings = new DevInternalSettings(applicationContext, this); - mDevServerHelper = new DevServerHelper( - mDevSettings, - new DevServerHelper.PackagerCommandListener() { - @Override - public void onReload() { - UiThreadUtil.runOnUiThread(new Runnable() { - @Override - public void run() { - handleReloadJS(); - } - }); - } - }); + mDevServerHelper = new DevServerHelper(mDevSettings); // Prepare shake gesture detector (will be started/stopped from #reload) mShakeDetector = new ShakeDetector(new ShakeDetector.ShakeListener() { @@ -237,8 +225,11 @@ public class DevSupportManagerImpl implements DevSupportManager { if (e instanceof JSException) { FLog.e(ReactConstants.TAG, "Exception in native call from JS", e); // TODO #11638796: convert the stack into something useful - showNewError(e.getMessage() + "\n\n" + ((JSException) e).getStack(), new StackFrame[] {}, - JSEXCEPTION_ERROR_COOKIE, ErrorType.JS); + showNewError( + e.getMessage() + "\n\n" + ((JSException) e).getStack(), + new StackFrame[] {}, + JSEXCEPTION_ERROR_COOKIE, + ErrorType.JS); } else { showNewJavaError(e.getMessage(), e); } @@ -388,7 +379,7 @@ public class DevSupportManagerImpl implements DevSupportManager { } }); options.put( - mApplicationContext.getString(R.string.catalyst_element_inspector), + mApplicationContext.getString(R.string.catalyst_element_inspector), new DevOptionHandler() { @Override public void onOptionSelected() { @@ -674,6 +665,16 @@ public class DevSupportManagerImpl implements DevSupportManager { return mLastErrorStack; } + @Override + public void onPackagerReloadCommand() { + UiThreadUtil.runOnUiThread(new Runnable() { + @Override + public void run() { + handleReloadJS(); + } + }); + } + private void updateLastErrorInfo( final String message, final StackFrame[] stack, @@ -802,6 +803,7 @@ public class DevSupportManagerImpl implements DevSupportManager { mIsReceiverRegistered = true; } + mDevServerHelper.openPackagerConnection(this); if (mDevSettings.isReloadOnJSChangeEnabled()) { mDevServerHelper.startPollingOnChangeEndpoint( new DevServerHelper.OnServerContentChangeListener() { @@ -841,6 +843,7 @@ public class DevSupportManagerImpl implements DevSupportManager { mDevOptionsDialog.dismiss(); } + mDevServerHelper.closePackagerConnection(); mDevServerHelper.stopPollingOnChangeEndpoint(); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/JSPackagerWebSocketClient.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/JSPackagerWebSocketClient.java index fd5f93bc0..0a3f96951 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/JSPackagerWebSocketClient.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/JSPackagerWebSocketClient.java @@ -40,6 +40,7 @@ public class JSPackagerWebSocketClient implements WebSocketListener { private final String mUrl; private final Handler mHandler; + private boolean mClosed = false; private boolean mSuppressConnectionErrors; public interface JSPackagerCallback { @@ -57,6 +58,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener { } public void connect() { + if (mClosed) { + throw new IllegalStateException("Can't connect closed client"); + } OkHttpClient httpClient = new OkHttpClient.Builder() .connectTimeout(10, TimeUnit.SECONDS) .writeTimeout(10, TimeUnit.SECONDS) @@ -69,6 +73,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener { } private void reconnect() { + if (mClosed) { + throw new IllegalStateException("Can't reconnect closed client"); + } if (!mSuppressConnectionErrors) { FLog.w(TAG, "Couldn't connect to packager, will silently retry"); mSuppressConnectionErrors = true; @@ -77,12 +84,21 @@ public class JSPackagerWebSocketClient implements WebSocketListener { new Runnable() { @Override public void run() { - connect(); + // check that we haven't been closed in the meantime + if (!mClosed) { + connect(); + } } - }, RECONNECT_DELAY_MS); + }, + RECONNECT_DELAY_MS); } public void closeQuietly() { + mClosed = true; + closeWebSocketQuietly(); + } + + private void closeWebSocketQuietly() { if (mWebSocket != null) { try { mWebSocket.close(1000, "End of session"); @@ -151,7 +167,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener { if (mWebSocket != null) { abort("Websocket exception", e); } - reconnect(); + if (!mClosed) { + reconnect(); + } } @Override @@ -163,7 +181,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener { @Override public void onClose(int code, String reason) { mWebSocket = null; - reconnect(); + if (!mClosed) { + reconnect(); + } } @Override @@ -173,6 +193,6 @@ public class JSPackagerWebSocketClient implements WebSocketListener { private void abort(String message, Throwable cause) { FLog.e(TAG, "Error occurred, shutting down websocket connection: " + message, cause); - closeQuietly(); + closeWebSocketQuietly(); } }