Skip to content

Commit

Permalink
Actually close packager websocket connection when destroying instance
Browse files Browse the repository at this point in the history 
Summary:
This is just D3835023 again since it got reverted, plus the fix from D3841918 baked in. Here's the old summary:

We never actually closed the websocket connection. Furthermore, upon calling `closeQuietly()`, `onClose()` is called, which does `reconnect()`. 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: D3849353

fbshipit-source-id: e1ce5e2d5840bfbd42a13043c3cc8c617e9fa64a
  • Loading branch information
@foghina
foghina authored and Facebook Github Bot 8 committed Sep 12, 2016
1 parent 84ce9d9 commit 16bdbee
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 42 deletions.
65 changes: 46 additions & 19 deletions ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,15 @@

package com.facebook.react.devsupport;

import javax.annotation.Nullable;

import java.io.File;
import java.io.IOException;
import java.util.Locale;
import java.util.concurrent.TimeUnit;

import android.content.Context;
import android.os.AsyncTask;
import android.os.Handler;
import android.text.TextUtils;

Expand All @@ -20,13 +28,6 @@
import com.facebook.react.common.network.OkHttpCallUtil;
import com.facebook.react.modules.systeminfo.AndroidInfoHelpers;

import java.io.File;
import java.io.IOException;
import java.util.Locale;
import java.util.concurrent.TimeUnit;

import javax.annotation.Nullable;

import okhttp3.Call;
import okhttp3.Callback;
import okhttp3.ConnectionPool;
Expand Down Expand Up @@ -79,7 +80,7 @@ public interface OnServerContentChangeListener {
}

public interface PackagerCommandListener {
void onReload();
void onPackagerReloadCommand();
}

public interface PackagerStatusCallback {
Expand All @@ -88,15 +89,15 @@ public interface PackagerStatusCallback {

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)
Expand All @@ -105,16 +106,42 @@ public DevServerHelper(DevInternalSettings settings, final PackagerCommandListen
.build();

mRestartOnChangePollingHandler = new Handler();
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();
}

public void openPackagerConnection(final PackagerCommandListener commandListener) {
if (mPackagerConnection != null) {
FLog.w(ReactConstants.TAG, "Packager connection already open, nooping.");
return;
}
new AsyncTask<Void, Void, Void>() {
@Override
protected Void doInBackground(Void... params) {
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.onPackagerReloadCommand();
}
}
}
});
mPackagerConnection.connect();
});
mPackagerConnection.connect();
return null;
}
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
}

public void closePackagerConnection() {
new AsyncTask<Void, Void, Void>() {
@Override
protected Void doInBackground(Void... params) {
if (mPackagerConnection != null) {
mPackagerConnection.closeQuietly();
mPackagerConnection = null;
}
return null;
}
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
}

/** Intent action for reloading the JS */
Expand Down
39 changes: 21 additions & 18 deletions ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
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;

Expand Down Expand Up @@ -82,7 +83,7 @@
* {@code <activity android:name="com.facebook.react.devsupport.DevSettingsActivity"/>}
* {@code <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>}
*/
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;
Expand Down Expand Up @@ -120,7 +121,6 @@ private static enum ErrorType {
private int mLastErrorCookie = 0;
private @Nullable ErrorType mLastErrorType;


private static class JscProfileTask extends AsyncTask<String, Void, Void> {
private static final MediaType JSON =
MediaType.parse("application/json; charset=utf-8");
Expand Down Expand Up @@ -178,19 +178,7 @@ public DevSupportManagerImpl(
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() {
Expand Down Expand Up @@ -237,8 +225,11 @@ public void handleException(Exception e) {
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);
}
Expand Down Expand Up @@ -388,7 +379,7 @@ public void onOptionSelected() {
}
});
options.put(
mApplicationContext.getString(R.string.catalyst_element_inspector),
mApplicationContext.getString(R.string.catalyst_element_inspector),
new DevOptionHandler() {
@Override
public void onOptionSelected() {
Expand Down Expand Up @@ -674,6 +665,16 @@ public void isPackagerRunning(DevServerHelper.PackagerStatusCallback callback) {
return mLastErrorStack;
}

@Override
public void onPackagerReloadCommand() {
UiThreadUtil.runOnUiThread(new Runnable() {
@Override
public void run() {
handleReloadJS();
}
});
}

private void updateLastErrorInfo(
final String message,
final StackFrame[] stack,
Expand Down Expand Up @@ -802,6 +803,7 @@ private void reload() {
mIsReceiverRegistered = true;
}

mDevServerHelper.openPackagerConnection(this);
if (mDevSettings.isReloadOnJSChangeEnabled()) {
mDevServerHelper.startPollingOnChangeEndpoint(
new DevServerHelper.OnServerContentChangeListener() {
Expand Down Expand Up @@ -841,6 +843,7 @@ public void onServerContentChanged() {
mDevOptionsDialog.dismiss();
}

mDevServerHelper.closePackagerConnection();
mDevServerHelper.stopPollingOnChangeEndpoint();
}
}
Expand Down
30 changes: 25 additions & 5 deletions ReactAndroid/src/main/java/com/facebook/react/devsupport/JSPackagerWebSocketClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -57,6 +58,9 @@ public JSPackagerWebSocketClient(String url, JSPackagerCallback callback) {
}

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)
Expand All @@ -69,6 +73,9 @@ public void connect() {
}

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;
Expand All @@ -77,12 +84,21 @@ private void reconnect() {
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");
Expand Down Expand Up @@ -151,7 +167,9 @@ public void onFailure(IOException e, Response response) {
if (mWebSocket != null) {
abort("Websocket exception", e);
}
reconnect();
if (!mClosed) {
reconnect();
}
}

@Override
Expand All @@ -163,7 +181,9 @@ public void onOpen(WebSocket webSocket, Response response) {
@Override
public void onClose(int code, String reason) {
mWebSocket = null;
reconnect();
if (!mClosed) {
reconnect();
}
}

@Override
Expand All @@ -173,6 +193,6 @@ public void onPong(Buffer payload) {

private void abort(String message, Throwable cause) {
FLog.e(TAG, "Error occurred, shutting down websocket connection: " + message, cause);
closeQuietly();
closeWebSocketQuietly();
}
}

0 comments on commit 16bdbee

Please sign in to comment.