Skip to content

Commit 6d9096f

Browse files
lexsfacebook-github-bot-5
lexs
authored and
facebook-github-bot-5
committed
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
1 parent fc98956 commit 6d9096f

8 files changed

Lines changed: 152 additions & 59 deletions

‎ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java‎

Lines changed: 61 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,8 @@
108108
new ReactInstanceDevCommandsHandler() {
109109

110110
@Override
111-
public void onReloadWithJSDebugger(JavaJSExecutor jsExecutor) {
112-
ReactInstanceManagerImpl.this.onReloadWithJSDebugger(jsExecutor);
111+
public void onReloadWithJSDebugger(JavaJSExecutor.Factory jsExecutorFactory) {
112+
ReactInstanceManagerImpl.this.onReloadWithJSDebugger(jsExecutorFactory);
113113
}
114114

115115
@Override
@@ -132,18 +132,18 @@ public void invokeDefaultOnBackPressed() {
132132
};
133133

134134
private class ReactContextInitParams {
135-
private final JavaScriptExecutor mJsExecutor;
135+
private final JavaScriptExecutor.Factory mJsExecutorFactory;
136136
private final JSBundleLoader mJsBundleLoader;
137137

138138
public ReactContextInitParams(
139-
JavaScriptExecutor jsExecutor,
139+
JavaScriptExecutor.Factory jsExecutorFactory,
140140
JSBundleLoader jsBundleLoader) {
141-
mJsExecutor = Assertions.assertNotNull(jsExecutor);
141+
mJsExecutorFactory = Assertions.assertNotNull(jsExecutorFactory);
142142
mJsBundleLoader = Assertions.assertNotNull(jsBundleLoader);
143143
}
144144

145-
public JavaScriptExecutor getJsExecutor() {
146-
return mJsExecutor;
145+
public JavaScriptExecutor.Factory getJsExecutorFactory() {
146+
return mJsExecutorFactory;
147147
}
148148

149149
public JSBundleLoader getJsBundleLoader() {
@@ -156,8 +156,7 @@ public JSBundleLoader getJsBundleLoader() {
156156
* be executing one at time, see {@link #recreateReactContextInBackground()}.
157157
*/
158158
private final class ReactContextInitAsyncTask extends
159-
AsyncTask<ReactContextInitParams, Void, ReactApplicationContext> {
160-
159+
AsyncTask<ReactContextInitParams, Void, Result<ReactApplicationContext>> {
161160
@Override
162161
protected void onPreExecute() {
163162
if (mCurrentReactContext != null) {
@@ -167,29 +166,70 @@ protected void onPreExecute() {
167166
}
168167

169168
@Override
170-
protected ReactApplicationContext doInBackground(ReactContextInitParams... params) {
169+
protected Result<ReactApplicationContext> doInBackground(ReactContextInitParams... params) {
171170
Assertions.assertCondition(params != null && params.length > 0 && params[0] != null);
172-
return createReactContext(params[0].getJsExecutor(), params[0].getJsBundleLoader());
171+
try {
172+
JavaScriptExecutor jsExecutor = params[0].getJsExecutorFactory().create();
173+
return Result.of(createReactContext(jsExecutor, params[0].getJsBundleLoader()));
174+
} catch (Exception e) {
175+
// Pass exception to onPostExecute() so it can be handled on the main thread
176+
return Result.of(e);
177+
}
173178
}
174179

175180
@Override
176-
protected void onPostExecute(ReactApplicationContext reactContext) {
181+
protected void onPostExecute(Result<ReactApplicationContext> result) {
177182
try {
178-
setupReactContext(reactContext);
183+
setupReactContext(result.get());
184+
} catch (Exception e) {
185+
mDevSupportManager.handleException(e);
179186
} finally {
180187
mIsContextInitAsyncTaskRunning = false;
181188
}
182189

183190
// Handle enqueued request to re-initialize react context.
184191
if (mPendingReactContextInitParams != null) {
185192
recreateReactContextInBackground(
186-
mPendingReactContextInitParams.getJsExecutor(),
193+
mPendingReactContextInitParams.getJsExecutorFactory(),
187194
mPendingReactContextInitParams.getJsBundleLoader());
188195
mPendingReactContextInitParams = null;
189196
}
190197
}
191198
}
192199

200+
private static class Result<T> {
201+
@Nullable private final T mResult;
202+
@Nullable private final Exception mException;
203+
204+
public static <T, U extends T> Result<T> of(U result) {
205+
return new Result<T>(result);
206+
}
207+
208+
public static <T> Result<T> of(Exception exception) {
209+
return new Result<>(exception);
210+
}
211+
212+
private Result(T result) {
213+
mException = null;
214+
mResult = result;
215+
}
216+
217+
private Result(Exception exception) {
218+
mException = exception;
219+
mResult = null;
220+
}
221+
222+
public T get() throws Exception {
223+
if (mException != null) {
224+
throw mException;
225+
}
226+
227+
Assertions.assertNotNull(mResult);
228+
229+
return mResult;
230+
}
231+
}
232+
193233
/* package */ ReactInstanceManagerImpl(
194234
Context applicationContext,
195235
@Nullable String jsBundleFile,
@@ -311,7 +351,7 @@ public void run() {
311351

312352
private void recreateReactContextInBackgroundFromBundleFile() {
313353
recreateReactContextInBackground(
314-
new JSCJavaScriptExecutor(),
354+
new JSCJavaScriptExecutor.Factory(),
315355
JSBundleLoader.createFileLoader(mApplicationContext, mJSBundleFile));
316356
}
317357

@@ -504,28 +544,29 @@ public void addReactInstanceEventListener(ReactInstanceEventListener listener) {
504544
return mCurrentReactContext;
505545
}
506546

507-
private void onReloadWithJSDebugger(JavaJSExecutor jsExecutor) {
547+
private void onReloadWithJSDebugger(JavaJSExecutor.Factory jsExecutorFactory) {
508548
recreateReactContextInBackground(
509-
new ProxyJavaScriptExecutor(jsExecutor),
549+
new ProxyJavaScriptExecutor.Factory(jsExecutorFactory),
510550
JSBundleLoader.createRemoteDebuggerBundleLoader(
511551
mDevSupportManager.getJSBundleURLForRemoteDebugging(),
512552
mDevSupportManager.getSourceUrl()));
513553
}
514554

515555
private void onJSBundleLoadedFromServer() {
516556
recreateReactContextInBackground(
517-
new JSCJavaScriptExecutor(),
557+
new JSCJavaScriptExecutor.Factory(),
518558
JSBundleLoader.createCachedBundleFromNetworkLoader(
519559
mDevSupportManager.getSourceUrl(),
520560
mDevSupportManager.getDownloadedJSBundleFile()));
521561
}
522562

523563
private void recreateReactContextInBackground(
524-
JavaScriptExecutor jsExecutor,
564+
JavaScriptExecutor.Factory jsExecutorFactory,
525565
JSBundleLoader jsBundleLoader) {
526566
UiThreadUtil.assertOnUiThread();
527567

528-
ReactContextInitParams initParams = new ReactContextInitParams(jsExecutor, jsBundleLoader);
568+
ReactContextInitParams initParams =
569+
new ReactContextInitParams(jsExecutorFactory, jsBundleLoader);
529570
if (!mIsContextInitAsyncTaskRunning) {
530571
// No background task to create react context is currently running, create and execute one.
531572
ReactContextInitAsyncTask initTask = new ReactContextInitAsyncTask();

‎ReactAndroid/src/main/java/com/facebook/react/bridge/JSCJavaScriptExecutor.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,12 @@
1414

1515
@DoNotStrip
1616
public class JSCJavaScriptExecutor extends JavaScriptExecutor {
17+
public static class Factory implements JavaScriptExecutor.Factory {
18+
@Override
19+
public JavaScriptExecutor create() throws Exception {
20+
return new JSCJavaScriptExecutor();
21+
}
22+
}
1723

1824
static {
1925
SoLoader.loadLibrary(ReactBridge.REACT_NATIVE_LIB);

‎ReactAndroid/src/main/java/com/facebook/react/bridge/JavaJSExecutor.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,11 @@
1818
*/
1919
@DoNotStrip
2020
public interface JavaJSExecutor {
21-
public static class ProxyExecutorException extends Exception {
21+
interface Factory {
22+
JavaJSExecutor create() throws Exception;
23+
}
24+
25+
class ProxyExecutorException extends Exception {
2226
public ProxyExecutorException(Throwable cause) {
2327
super(cause);
2428
}

‎ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptExecutor.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414

1515
@DoNotStrip
1616
public abstract class JavaScriptExecutor extends Countable {
17+
public interface Factory {
18+
JavaScriptExecutor create() throws Exception;
19+
}
1720

1821
/**
1922
* Close this executor and cleanup any resources that it was using. No further calls are

‎ReactAndroid/src/main/java/com/facebook/react/bridge/ProxyJavaScriptExecutor.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,18 @@
2424
*/
2525
@DoNotStrip
2626
public class ProxyJavaScriptExecutor extends JavaScriptExecutor {
27+
public static class Factory implements JavaScriptExecutor.Factory {
28+
private final JavaJSExecutor.Factory mJavaJSExecutorFactory;
29+
30+
public Factory(JavaJSExecutor.Factory javaJSExecutorFactory) {
31+
mJavaJSExecutorFactory = javaJSExecutorFactory;
32+
}
33+
34+
@Override
35+
public JavaScriptExecutor create() throws Exception {
36+
return new ProxyJavaScriptExecutor(mJavaJSExecutorFactory.create());
37+
}
38+
}
2739

2840
static {
2941
SoLoader.loadLibrary(ReactBridge.REACT_NATIVE_LIB);

‎ReactAndroid/src/main/java/com/facebook/react/bridge/WebsocketJavaScriptExecutor.java‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,10 @@
1313

1414
import java.util.HashMap;
1515
import java.util.concurrent.Semaphore;
16-
import java.util.concurrent.TimeUnit;
1716
import java.util.concurrent.atomic.AtomicInteger;
1817

1918
import android.os.Handler;
19+
import android.os.Looper;
2020

2121
import com.facebook.infer.annotation.Assertions;
2222

@@ -97,9 +97,13 @@ private void connectInternal(
9797
String webSocketServerUrl,
9898
final JSExecutorConnectCallback callback) {
9999
final JSDebuggerWebSocketClient client = new JSDebuggerWebSocketClient();
100-
final Handler timeoutHandler = new Handler();
100+
final Handler timeoutHandler = new Handler(Looper.getMainLooper());
101101
client.connect(
102102
webSocketServerUrl, new JSDebuggerWebSocketClient.JSDebuggerCallback() {
103+
// It's possible that both callbacks can fire on an error so make sure we only
104+
// dispatch results once to our callback.
105+
private boolean didSendResult = false;
106+
103107
@Override
104108
public void onSuccess(@Nullable String response) {
105109
client.prepareJSRuntime(
@@ -108,20 +112,30 @@ public void onSuccess(@Nullable String response) {
108112
public void onSuccess(@Nullable String response) {
109113
timeoutHandler.removeCallbacksAndMessages(null);
110114
mWebSocketClient = client;
111-
callback.onSuccess();
115+
if (!didSendResult) {
116+
callback.onSuccess();
117+
didSendResult = true;
118+
}
112119
}
113120

114121
@Override
115122
public void onFailure(Throwable cause) {
116123
timeoutHandler.removeCallbacksAndMessages(null);
117-
callback.onFailure(cause);
124+
if (!didSendResult) {
125+
callback.onFailure(cause);
126+
didSendResult = true;
127+
}
118128
}
119129
});
120130
}
121131

122132
@Override
123133
public void onFailure(Throwable cause) {
124-
callback.onFailure(cause);
134+
timeoutHandler.removeCallbacksAndMessages(null);
135+
if (!didSendResult) {
136+
callback.onFailure(cause);
137+
didSendResult = true;
138+
}
125139
}
126140
});
127141
timeoutHandler.postDelayed(

‎ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManager.java‎

Lines changed: 45 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@
1515
import java.io.IOException;
1616
import java.util.LinkedHashMap;
1717
import java.util.Locale;
18+
import java.util.concurrent.ExecutionException;
19+
import java.util.concurrent.TimeUnit;
20+
import java.util.concurrent.TimeoutException;
1821

1922
import android.app.AlertDialog;
2023
import android.app.ProgressDialog;
@@ -35,14 +38,15 @@
3538
import com.facebook.infer.annotation.Assertions;
3639
import com.facebook.react.R;
3740
import com.facebook.react.bridge.CatalystInstance;
41+
import com.facebook.react.bridge.JavaJSExecutor;
3842
import com.facebook.react.bridge.NativeModuleCallExceptionHandler;
39-
import com.facebook.react.bridge.ProxyJavaScriptExecutor;
4043
import com.facebook.react.bridge.ReactContext;
4144
import com.facebook.react.bridge.ReadableArray;
4245
import com.facebook.react.bridge.UiThreadUtil;
4346
import com.facebook.react.bridge.WebsocketJavaScriptExecutor;
4447
import com.facebook.react.common.ReactConstants;
4548
import com.facebook.react.common.ShakeDetector;
49+
import com.facebook.react.common.futures.SimpleSettableFuture;
4650
import com.facebook.react.devsupport.StackTraceHelper.StackFrame;
4751
import com.facebook.react.modules.debug.DeveloperSettings;
4852

@@ -534,38 +538,47 @@ private void reloadJSInProxyMode(final ProgressDialog progressDialog) {
534538
// anyway
535539
mDevServerHelper.launchChromeDevtools();
536540

537-
final WebsocketJavaScriptExecutor webSocketJSExecutor = new WebsocketJavaScriptExecutor();
538-
webSocketJSExecutor.connect(
539-
mDevServerHelper.getWebsocketProxyURL(),
540-
new WebsocketJavaScriptExecutor.JSExecutorConnectCallback() {
541-
@Override
542-
public void onSuccess() {
543-
progressDialog.dismiss();
544-
UiThreadUtil.runOnUiThread(
545-
new Runnable() {
546-
@Override
547-
public void run() {
548-
mReactInstanceCommandsHandler.onReloadWithJSDebugger(
549-
webSocketJSExecutor);
550-
}
551-
});
552-
}
541+
JavaJSExecutor.Factory factory = new JavaJSExecutor.Factory() {
542+
@Override
543+
public JavaJSExecutor create() throws Exception {
544+
WebsocketJavaScriptExecutor executor = new WebsocketJavaScriptExecutor();
545+
SimpleSettableFuture<Boolean> future = new SimpleSettableFuture<>();
546+
executor.connect(
547+
mDevServerHelper.getWebsocketProxyURL(),
548+
getExecutorConnectCallback(progressDialog, future));
549+
// TODO(t9349129) Don't use timeout
550+
try {
551+
future.get(90, TimeUnit.SECONDS);
552+
return executor;
553+
} catch (ExecutionException e) {
554+
throw (Exception) e.getCause();
555+
} catch (InterruptedException | TimeoutException e) {
556+
throw new RuntimeException(e);
557+
}
558+
}
559+
};
560+
mReactInstanceCommandsHandler.onReloadWithJSDebugger(factory);
561+
}
553562

554-
@Override
555-
public void onFailure(final Throwable cause) {
556-
progressDialog.dismiss();
557-
FLog.e(ReactConstants.TAG, "Unable to connect to remote debugger", cause);
558-
UiThreadUtil.runOnUiThread(
559-
new Runnable() {
560-
@Override
561-
public void run() {
562-
showNewJavaError(
563-
mApplicationContext.getString(R.string.catalyst_remotedbg_error),
564-
cause);
565-
}
566-
});
567-
}
568-
});
563+
private WebsocketJavaScriptExecutor.JSExecutorConnectCallback getExecutorConnectCallback(
564+
final ProgressDialog progressDialog,
565+
final SimpleSettableFuture<Boolean> future) {
566+
return new WebsocketJavaScriptExecutor.JSExecutorConnectCallback() {
567+
@Override
568+
public void onSuccess() {
569+
future.set(true);
570+
progressDialog.dismiss();
571+
}
572+
573+
@Override
574+
public void onFailure(final Throwable cause) {
575+
progressDialog.dismiss();
576+
FLog.e(ReactConstants.TAG, "Unable to connect to remote debugger", cause);
577+
future.setException(
578+
new IOException(
579+
mApplicationContext.getString(R.string.catalyst_remotedbg_error), cause));
580+
}
581+
};
569582
}
570583

571584
private void reloadJSFromServer(final ProgressDialog progressDialog) {

‎ReactAndroid/src/main/java/com/facebook/react/devsupport/ReactInstanceDevCommandsHandler.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public interface ReactInstanceDevCommandsHandler {
2020
/**
2121
* Request react instance recreation with JS debugging enabled.
2222
*/
23-
void onReloadWithJSDebugger(JavaJSExecutor proxyExecutor);
23+
void onReloadWithJSDebugger(JavaJSExecutor.Factory proxyExecutorFactory);
2424

2525
/**
2626
* Notify react instance manager about new JS bundle version downloaded from the server.

0 commit comments

Comments
 (0)