From 6d9096fb3f915117703c19c8cee7acd9d11bc378 Mon Sep 17 00:00:00 2001 From: Alexander Blom Date: Tue, 5 Jan 2016 09:26:54 -0800 Subject: [PATCH] Fix deadlock when recreating websocket connection Summary: When you reload code while using the Chrome debugger we used to create a new websocket connection before closing the old one. This sometimes would cause a in-flight call to not get a response. This in turn would deadlock the JS thread because we try to shut it down before killing the websocket connection. This change instead makes sure to close the old connection before creating a new one. This is done by using a factory for creating the JavascriptExecutor so we can defer the creation until after the old Bridge has been torn down. public Reviewed By: astreet Differential Revision: D2735011 fb-gh-sync-id: 0ce0f35abaeef5457bad8d6b8d10122281192af4 --- .../react/ReactInstanceManagerImpl.java | 81 ++++++++++++++----- .../react/bridge/JSCJavaScriptExecutor.java | 6 ++ .../facebook/react/bridge/JavaJSExecutor.java | 6 +- .../react/bridge/JavaScriptExecutor.java | 3 + .../react/bridge/ProxyJavaScriptExecutor.java | 12 +++ .../bridge/WebsocketJavaScriptExecutor.java | 24 ++++-- .../react/devsupport/DevSupportManager.java | 77 ++++++++++-------- .../ReactInstanceDevCommandsHandler.java | 2 +- 8 files changed, 152 insertions(+), 59 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java index 0baef21f2..780cdb123 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java @@ -108,8 +108,8 @@ import com.facebook.systrace.Systrace; new ReactInstanceDevCommandsHandler() { @Override - public void onReloadWithJSDebugger(JavaJSExecutor jsExecutor) { - ReactInstanceManagerImpl.this.onReloadWithJSDebugger(jsExecutor); + public void onReloadWithJSDebugger(JavaJSExecutor.Factory jsExecutorFactory) { + ReactInstanceManagerImpl.this.onReloadWithJSDebugger(jsExecutorFactory); } @Override @@ -132,18 +132,18 @@ import com.facebook.systrace.Systrace; }; private class ReactContextInitParams { - private final JavaScriptExecutor mJsExecutor; + private final JavaScriptExecutor.Factory mJsExecutorFactory; private final JSBundleLoader mJsBundleLoader; public ReactContextInitParams( - JavaScriptExecutor jsExecutor, + JavaScriptExecutor.Factory jsExecutorFactory, JSBundleLoader jsBundleLoader) { - mJsExecutor = Assertions.assertNotNull(jsExecutor); + mJsExecutorFactory = Assertions.assertNotNull(jsExecutorFactory); mJsBundleLoader = Assertions.assertNotNull(jsBundleLoader); } - public JavaScriptExecutor getJsExecutor() { - return mJsExecutor; + public JavaScriptExecutor.Factory getJsExecutorFactory() { + return mJsExecutorFactory; } public JSBundleLoader getJsBundleLoader() { @@ -156,8 +156,7 @@ import com.facebook.systrace.Systrace; * be executing one at time, see {@link #recreateReactContextInBackground()}. */ private final class ReactContextInitAsyncTask extends - AsyncTask { - + AsyncTask> { @Override protected void onPreExecute() { if (mCurrentReactContext != null) { @@ -167,15 +166,23 @@ import com.facebook.systrace.Systrace; } @Override - protected ReactApplicationContext doInBackground(ReactContextInitParams... params) { + protected Result doInBackground(ReactContextInitParams... params) { Assertions.assertCondition(params != null && params.length > 0 && params[0] != null); - return createReactContext(params[0].getJsExecutor(), params[0].getJsBundleLoader()); + try { + JavaScriptExecutor jsExecutor = params[0].getJsExecutorFactory().create(); + return Result.of(createReactContext(jsExecutor, params[0].getJsBundleLoader())); + } catch (Exception e) { + // Pass exception to onPostExecute() so it can be handled on the main thread + return Result.of(e); + } } @Override - protected void onPostExecute(ReactApplicationContext reactContext) { + protected void onPostExecute(Result result) { try { - setupReactContext(reactContext); + setupReactContext(result.get()); + } catch (Exception e) { + mDevSupportManager.handleException(e); } finally { mIsContextInitAsyncTaskRunning = false; } @@ -183,13 +190,46 @@ import com.facebook.systrace.Systrace; // Handle enqueued request to re-initialize react context. if (mPendingReactContextInitParams != null) { recreateReactContextInBackground( - mPendingReactContextInitParams.getJsExecutor(), + mPendingReactContextInitParams.getJsExecutorFactory(), mPendingReactContextInitParams.getJsBundleLoader()); mPendingReactContextInitParams = null; } } } + private static class Result { + @Nullable private final T mResult; + @Nullable private final Exception mException; + + public static Result of(U result) { + return new Result(result); + } + + public static Result of(Exception exception) { + return new Result<>(exception); + } + + private Result(T result) { + mException = null; + mResult = result; + } + + private Result(Exception exception) { + mException = exception; + mResult = null; + } + + public T get() throws Exception { + if (mException != null) { + throw mException; + } + + Assertions.assertNotNull(mResult); + + return mResult; + } + } + /* package */ ReactInstanceManagerImpl( Context applicationContext, @Nullable String jsBundleFile, @@ -311,7 +351,7 @@ import com.facebook.systrace.Systrace; private void recreateReactContextInBackgroundFromBundleFile() { recreateReactContextInBackground( - new JSCJavaScriptExecutor(), + new JSCJavaScriptExecutor.Factory(), JSBundleLoader.createFileLoader(mApplicationContext, mJSBundleFile)); } @@ -504,9 +544,9 @@ import com.facebook.systrace.Systrace; return mCurrentReactContext; } - private void onReloadWithJSDebugger(JavaJSExecutor jsExecutor) { + private void onReloadWithJSDebugger(JavaJSExecutor.Factory jsExecutorFactory) { recreateReactContextInBackground( - new ProxyJavaScriptExecutor(jsExecutor), + new ProxyJavaScriptExecutor.Factory(jsExecutorFactory), JSBundleLoader.createRemoteDebuggerBundleLoader( mDevSupportManager.getJSBundleURLForRemoteDebugging(), mDevSupportManager.getSourceUrl())); @@ -514,18 +554,19 @@ import com.facebook.systrace.Systrace; private void onJSBundleLoadedFromServer() { recreateReactContextInBackground( - new JSCJavaScriptExecutor(), + new JSCJavaScriptExecutor.Factory(), JSBundleLoader.createCachedBundleFromNetworkLoader( mDevSupportManager.getSourceUrl(), mDevSupportManager.getDownloadedJSBundleFile())); } private void recreateReactContextInBackground( - JavaScriptExecutor jsExecutor, + JavaScriptExecutor.Factory jsExecutorFactory, JSBundleLoader jsBundleLoader) { UiThreadUtil.assertOnUiThread(); - ReactContextInitParams initParams = new ReactContextInitParams(jsExecutor, jsBundleLoader); + ReactContextInitParams initParams = + new ReactContextInitParams(jsExecutorFactory, jsBundleLoader); if (!mIsContextInitAsyncTaskRunning) { // No background task to create react context is currently running, create and execute one. ReactContextInitAsyncTask initTask = new ReactContextInitAsyncTask(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSCJavaScriptExecutor.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSCJavaScriptExecutor.java index d371cf5be..32b261930 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSCJavaScriptExecutor.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSCJavaScriptExecutor.java @@ -14,6 +14,12 @@ import com.facebook.soloader.SoLoader; @DoNotStrip public class JSCJavaScriptExecutor extends JavaScriptExecutor { + public static class Factory implements JavaScriptExecutor.Factory { + @Override + public JavaScriptExecutor create() throws Exception { + return new JSCJavaScriptExecutor(); + } + } static { SoLoader.loadLibrary(ReactBridge.REACT_NATIVE_LIB); diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaJSExecutor.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaJSExecutor.java index 3c8eb4574..5c1b01b22 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaJSExecutor.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaJSExecutor.java @@ -18,7 +18,11 @@ import com.facebook.proguard.annotations.DoNotStrip; */ @DoNotStrip public interface JavaJSExecutor { - public static class ProxyExecutorException extends Exception { + interface Factory { + JavaJSExecutor create() throws Exception; + } + + class ProxyExecutorException extends Exception { public ProxyExecutorException(Throwable cause) { super(cause); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptExecutor.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptExecutor.java index 2bc5e26c5..bdf7e4fc6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptExecutor.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptExecutor.java @@ -14,6 +14,9 @@ import com.facebook.proguard.annotations.DoNotStrip; @DoNotStrip public abstract class JavaScriptExecutor extends Countable { + public interface Factory { + JavaScriptExecutor create() throws Exception; + } /** * Close this executor and cleanup any resources that it was using. No further calls are diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ProxyJavaScriptExecutor.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ProxyJavaScriptExecutor.java index f9e58869b..80cd6f4b3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ProxyJavaScriptExecutor.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ProxyJavaScriptExecutor.java @@ -24,6 +24,18 @@ import com.facebook.proguard.annotations.DoNotStrip; */ @DoNotStrip public class ProxyJavaScriptExecutor extends JavaScriptExecutor { + public static class Factory implements JavaScriptExecutor.Factory { + private final JavaJSExecutor.Factory mJavaJSExecutorFactory; + + public Factory(JavaJSExecutor.Factory javaJSExecutorFactory) { + mJavaJSExecutorFactory = javaJSExecutorFactory; + } + + @Override + public JavaScriptExecutor create() throws Exception { + return new ProxyJavaScriptExecutor(mJavaJSExecutorFactory.create()); + } + } static { SoLoader.loadLibrary(ReactBridge.REACT_NATIVE_LIB); diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/WebsocketJavaScriptExecutor.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/WebsocketJavaScriptExecutor.java index 0fba2b0a3..cddadba00 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/WebsocketJavaScriptExecutor.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/WebsocketJavaScriptExecutor.java @@ -13,10 +13,10 @@ import javax.annotation.Nullable; import java.util.HashMap; import java.util.concurrent.Semaphore; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import android.os.Handler; +import android.os.Looper; import com.facebook.infer.annotation.Assertions; @@ -97,9 +97,13 @@ public class WebsocketJavaScriptExecutor implements JavaJSExecutor { String webSocketServerUrl, final JSExecutorConnectCallback callback) { final JSDebuggerWebSocketClient client = new JSDebuggerWebSocketClient(); - final Handler timeoutHandler = new Handler(); + final Handler timeoutHandler = new Handler(Looper.getMainLooper()); client.connect( webSocketServerUrl, new JSDebuggerWebSocketClient.JSDebuggerCallback() { + // It's possible that both callbacks can fire on an error so make sure we only + // dispatch results once to our callback. + private boolean didSendResult = false; + @Override public void onSuccess(@Nullable String response) { client.prepareJSRuntime( @@ -108,20 +112,30 @@ public class WebsocketJavaScriptExecutor implements JavaJSExecutor { public void onSuccess(@Nullable String response) { timeoutHandler.removeCallbacksAndMessages(null); mWebSocketClient = client; - callback.onSuccess(); + if (!didSendResult) { + callback.onSuccess(); + didSendResult = true; + } } @Override public void onFailure(Throwable cause) { timeoutHandler.removeCallbacksAndMessages(null); - callback.onFailure(cause); + if (!didSendResult) { + callback.onFailure(cause); + didSendResult = true; + } } }); } @Override public void onFailure(Throwable cause) { - callback.onFailure(cause); + timeoutHandler.removeCallbacksAndMessages(null); + if (!didSendResult) { + callback.onFailure(cause); + didSendResult = true; + } } }); timeoutHandler.postDelayed( diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManager.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManager.java index bcc094062..ce60e861c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManager.java @@ -15,6 +15,9 @@ import java.io.File; import java.io.IOException; import java.util.LinkedHashMap; import java.util.Locale; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import android.app.AlertDialog; import android.app.ProgressDialog; @@ -35,14 +38,15 @@ import com.facebook.common.logging.FLog; import com.facebook.infer.annotation.Assertions; import com.facebook.react.R; import com.facebook.react.bridge.CatalystInstance; +import com.facebook.react.bridge.JavaJSExecutor; import com.facebook.react.bridge.NativeModuleCallExceptionHandler; -import com.facebook.react.bridge.ProxyJavaScriptExecutor; import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.bridge.WebsocketJavaScriptExecutor; import com.facebook.react.common.ReactConstants; import com.facebook.react.common.ShakeDetector; +import com.facebook.react.common.futures.SimpleSettableFuture; import com.facebook.react.devsupport.StackTraceHelper.StackFrame; import com.facebook.react.modules.debug.DeveloperSettings; @@ -534,38 +538,47 @@ public class DevSupportManager implements NativeModuleCallExceptionHandler { // anyway mDevServerHelper.launchChromeDevtools(); - final WebsocketJavaScriptExecutor webSocketJSExecutor = new WebsocketJavaScriptExecutor(); - webSocketJSExecutor.connect( - mDevServerHelper.getWebsocketProxyURL(), - new WebsocketJavaScriptExecutor.JSExecutorConnectCallback() { - @Override - public void onSuccess() { - progressDialog.dismiss(); - UiThreadUtil.runOnUiThread( - new Runnable() { - @Override - public void run() { - mReactInstanceCommandsHandler.onReloadWithJSDebugger( - webSocketJSExecutor); - } - }); - } + JavaJSExecutor.Factory factory = new JavaJSExecutor.Factory() { + @Override + public JavaJSExecutor create() throws Exception { + WebsocketJavaScriptExecutor executor = new WebsocketJavaScriptExecutor(); + SimpleSettableFuture future = new SimpleSettableFuture<>(); + executor.connect( + mDevServerHelper.getWebsocketProxyURL(), + getExecutorConnectCallback(progressDialog, future)); + // TODO(t9349129) Don't use timeout + try { + future.get(90, TimeUnit.SECONDS); + return executor; + } catch (ExecutionException e) { + throw (Exception) e.getCause(); + } catch (InterruptedException | TimeoutException e) { + throw new RuntimeException(e); + } + } + }; + mReactInstanceCommandsHandler.onReloadWithJSDebugger(factory); + } - @Override - public void onFailure(final Throwable cause) { - progressDialog.dismiss(); - FLog.e(ReactConstants.TAG, "Unable to connect to remote debugger", cause); - UiThreadUtil.runOnUiThread( - new Runnable() { - @Override - public void run() { - showNewJavaError( - mApplicationContext.getString(R.string.catalyst_remotedbg_error), - cause); - } - }); - } - }); + private WebsocketJavaScriptExecutor.JSExecutorConnectCallback getExecutorConnectCallback( + final ProgressDialog progressDialog, + final SimpleSettableFuture future) { + return new WebsocketJavaScriptExecutor.JSExecutorConnectCallback() { + @Override + public void onSuccess() { + future.set(true); + progressDialog.dismiss(); + } + + @Override + public void onFailure(final Throwable cause) { + progressDialog.dismiss(); + FLog.e(ReactConstants.TAG, "Unable to connect to remote debugger", cause); + future.setException( + new IOException( + mApplicationContext.getString(R.string.catalyst_remotedbg_error), cause)); + } + }; } private void reloadJSFromServer(final ProgressDialog progressDialog) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/ReactInstanceDevCommandsHandler.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/ReactInstanceDevCommandsHandler.java index e97b16f7d..ce3639be8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/ReactInstanceDevCommandsHandler.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/ReactInstanceDevCommandsHandler.java @@ -20,7 +20,7 @@ public interface ReactInstanceDevCommandsHandler { /** * Request react instance recreation with JS debugging enabled. */ - void onReloadWithJSDebugger(JavaJSExecutor proxyExecutor); + void onReloadWithJSDebugger(JavaJSExecutor.Factory proxyExecutorFactory); /** * Notify react instance manager about new JS bundle version downloaded from the server.