Fix stale packager connections on Android

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.kt

@@ -16,6 +16,7 @@ import android.content.Context
 import android.net.Uri
 import android.os.AsyncTask
 import android.provider.Settings.Secure
+import androidx.annotation.VisibleForTesting
 import com.facebook.common.logging.FLog
 import com.facebook.react.bridge.ReactContext
 import com.facebook.react.common.ReactConstants
@@ -39,6 +40,10 @@ import java.io.UnsupportedEncodingException
 import java.security.MessageDigest
 import java.security.NoSuchAlgorithmException
 import java.util.Locale
+import java.util.concurrent.Executor
+import java.util.concurrent.LinkedBlockingQueue
+import java.util.concurrent.ThreadPoolExecutor
+import java.util.concurrent.TimeUnit
 import okhttp3.Call
 import okhttp3.Callback
 import okhttp3.OkHttpClient
@@ -73,7 +78,8 @@ public open class DevServerHelper(
 
     public fun onPackagerDevMenuCommand()
 
-    // Allow apps to provide listeners for custom packager commands.
+    // Allow apps to provide listeners for custom packager commands. Handlers must not block,
+    // because closing the connection waits for a running handler.
     public fun customCommandHandlers(): Map<String, RequestHandler>?
   }
 
@@ -87,7 +93,14 @@ public open class DevServerHelper(
   private val packagerStatusCheck: PackagerStatusCheck = PackagerStatusCheck(client)
   private val packageName: String = applicationContext.packageName
 
+  // Must stay single-threaded: open and close have to run in call order.
+  @VisibleForTesting
+  internal var packagerConnectionExecutor: Executor =
+      ThreadPoolExecutor(0, 1, 30, TimeUnit.SECONDS, LinkedBlockingQueue()) { runnable ->
+        Thread(runnable, PACKAGER_CONNECTION_THREAD_NAME)
+      }
   private var packagerClient: JSPackagerClient? = null
+  private var packagerClientHost: String? = null
   private var inspectorPackagerConnection: IInspectorPackagerConnection? = null
 
   /** Returns an opaque ID which is stable for the current combination of device and app, stable */
@@ -138,65 +151,62 @@ public open class DevServerHelper(
     get() = settings.isJSMinifyEnabled
 
   public fun openPackagerConnection(clientId: String?, commandListener: PackagerCommandListener) {
-    if (packagerClient != null) {
-      FLog.w(ReactConstants.TAG, "Packager connection already open, nooping.")
-      return
-    }
-    object : AsyncTask<Void, Void, Void>() {
-          @Deprecated("This needs to be rewritten to not use AsyncTasks")
-          override fun doInBackground(vararg backgroundParams: Void): Void? {
-            val handlers: MutableMap<String, RequestHandler> = mutableMapOf()
-            handlers["reload"] =
-                object : NotificationOnlyHandler() {
-                  override fun onNotification(params: Any?) {
-                    commandListener.onPackagerReloadCommand()
-                  }
-                }
-            handlers["devMenu"] =
-                object : NotificationOnlyHandler() {
-                  override fun onNotification(params: Any?) {
-                    commandListener.onPackagerDevMenuCommand()
-                  }
-                }
-            commandListener.customCommandHandlers()?.let { handlers.putAll(it) }
-
-            val onPackagerConnectedCallback: ReconnectingWebSocket.ConnectionCallback =
-                object : ReconnectingWebSocket.ConnectionCallback {
-                  override fun onConnected() {
-                    commandListener.onPackagerConnected()
-                  }
-
-                  override fun onDisconnected() {
-                    commandListener.onPackagerDisconnected()
-                  }
-                }
-
-            checkNotNull(clientId)
-            packagerClient =
-                JSPackagerClient(
-                        clientId,
-                        packagerConnectionSettings,
-                        handlers,
-                        onPackagerConnectedCallback,
-                    )
-                    .apply { init() }
+    val id = checkNotNull(clientId)
+    packagerConnectionExecutor.execute {
+      val host = packagerConnectionSettings.debugServerHost
+      packagerClient?.let { client ->
+        if (host == packagerClientHost) {
+          FLog.w(ReactConstants.TAG, "Packager connection already open, nooping.")
+          return@execute
+        }
+        // The dev server host changed without a close.
+        client.close()
+        packagerClient = null
+      }
+      val handlers: MutableMap<String, RequestHandler> = mutableMapOf()
+      handlers["reload"] =
+          object : NotificationOnlyHandler() {
+            override fun onNotification(params: Any?) {
+              commandListener.onPackagerReloadCommand()
+            }
+          }
+      handlers["devMenu"] =
+          object : NotificationOnlyHandler() {
+            override fun onNotification(params: Any?) {
+              commandListener.onPackagerDevMenuCommand()
+            }
+          }
+      commandListener.customCommandHandlers()?.let { handlers.putAll(it) }
 
-            return null
+      val onPackagerConnectedCallback: ReconnectingWebSocket.ConnectionCallback =
+          object : ReconnectingWebSocket.ConnectionCallback {
+            override fun onConnected() {
+              commandListener.onPackagerConnected()
+            }
+
+            override fun onDisconnected() {
+              commandListener.onPackagerDisconnected()
+            }
           }
-        }
-        .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR)
+
+      packagerClient =
+          JSPackagerClient(
+                  id,
+                  packagerConnectionSettings,
+                  handlers,
+                  onPackagerConnectedCallback,
+              )
+              .apply { init() }
+      packagerClientHost = host
+    }
   }
 
   public fun closePackagerConnection() {
-    object : AsyncTask<Void, Void, Void>() {
-          @Deprecated("This class needs to be rewritten to don't use AsyncTasks")
-          override fun doInBackground(vararg params: Void): Void? {
-            packagerClient?.close()
-            packagerClient = null
-            return null
-          }
-        }
-        .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR)
+    packagerConnectionExecutor.execute {
+      packagerClient?.close()
+      packagerClient = null
+      packagerClientHost = null
+    }
   }
 
   public fun openInspectorConnection() {
@@ -385,6 +395,7 @@ public open class DevServerHelper(
 
   private companion object {
     private const val DEBUGGER_MSG_DISABLE = "{ \"id\":1,\"method\":\"Debugger.disable\" }"
+    private const val PACKAGER_CONNECTION_THREAD_NAME = "ReactPackagerConnection"
 
     private fun getSHA256(string: String): String {
       val digest =

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.kt

@@ -52,6 +52,7 @@ import com.facebook.react.devsupport.DevServerHelper.PackagerCommandListener
 import com.facebook.react.devsupport.InspectorFlags.getFuseboxEnabled
 import com.facebook.react.devsupport.StackTraceHelper.convertJavaStackTrace
 import com.facebook.react.devsupport.StackTraceHelper.convertJsStackTrace
+import com.facebook.react.devsupport.inspector.DevSupportHttpClient
 import com.facebook.react.devsupport.inspector.TracingState
 import com.facebook.react.devsupport.inspector.TracingStateProvider
 import com.facebook.react.devsupport.interfaces.BundleLoadCallback
@@ -450,9 +451,24 @@ public abstract class DevSupportManagerBase(
             return@DevOptionHandler
           }
 
-          ChangeBundleLocationDialog.show(context, devSettings) { host: String ->
-            devSettings.packagerConnectionSettings.debugServerHost = host
-            handleReloadJS()
+          ChangeBundleLocationDialog.show(context, devSettings) { input: String ->
+            val host = input.trim()
+            // An empty host resets to the default. An invalid host would crash the connection.
+            if (host.isEmpty() || DevSupportHttpClient.isValidHost(host)) {
+              devSettings.packagerConnectionSettings.debugServerHost = host
+              devServerHelper.closePackagerConnection()
+              handleReloadJS()
+            } else {
+              Toast.makeText(
+                      applicationContext,
+                      applicationContext.getString(
+                          R.string.catalyst_change_bundle_location_invalid,
+                          host,
+                      ),
+                      Toast.LENGTH_LONG,
+                  )
+                  .show()
+            }
           }
         }
 

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/inspector/DevSupportHttpClient.kt

@@ -13,6 +13,7 @@ import com.facebook.react.modules.network.OkHttpClientProvider
 import java.util.concurrent.TimeUnit
 import okhttp3.ConnectionPool
 import okhttp3.Dispatcher
+import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 import okhttp3.OkHttpClient
 
 /**
@@ -68,4 +69,10 @@ internal object DevSupportHttpClient {
    * the host specifies port 443 explicitly (e.g. "example.com:443").
    */
   internal fun wsScheme(host: String): String = if (host.endsWith(":443")) "wss" else "ws"
+
+  /**
+   * Returns whether OkHttp can build a request for the given host, for example "localhost:8081".
+   */
+  internal fun isValidHost(host: String): Boolean =
+      "${httpScheme(host)}://$host/".toHttpUrlOrNull() != null
 }

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/packagerconnection/ReconnectingWebSocket.kt

@@ -77,16 +77,18 @@ public class ReconnectingWebSocket(
   }
 
   public fun closeQuietly() {
-    closed = true
-    closeWebSocketQuietly()
-    messageCallback = null
+    synchronized(this) {
+      closed = true
+      closeWebSocketQuietly()
+      messageCallback = null
+    }
 
     connectionCallback?.onDisconnected()
   }
 
   private fun closeWebSocketQuietly() {
     try {
-      webSocket?.close(1_000, "End of session")
+      webSocket?.close(CLOSE_NORMAL, CLOSE_REASON)
     } catch (e: Exception) {
       // swallow, no need to handle it here
     }
@@ -100,6 +102,10 @@ public class ReconnectingWebSocket(
 
   @Synchronized
   override fun onOpen(webSocket: WebSocket, response: Response) {
+    if (closed) {
+      webSocket.close(CLOSE_NORMAL, CLOSE_REASON)
+      return
+    }
     this.webSocket = webSocket
     suppressConnectionErrors = false
 
@@ -155,5 +161,7 @@ public class ReconnectingWebSocket(
     private val TAG: String = ReconnectingWebSocket::class.java.simpleName
 
     private const val RECONNECT_DELAY_MS = 2_000L
+    private const val CLOSE_NORMAL = 1_000
+    private const val CLOSE_REASON = "End of session"
   }
 }

packages/react-native/ReactAndroid/src/main/res/devsupport/values/strings.xml

@@ -8,6 +8,7 @@
   <string name="catalyst_change_bundle_location_instructions" project="catalyst" translatable="false">You can connect either via USB (localhost - default) or Wifi. If you connect via USB and running with a physical device, make sure you:\n 1. Connect your device via USB\n 2. Set the bundle location to `localhost:8081`\n 3. Run this command in your terminal:\n&#160;&#160;&#160;&#160;&#160;&#160;`%1$s`</string>
   <string name="catalyst_change_bundle_location_apply" project="catalyst" translatable="false">Apply Changes</string>
   <string name="catalyst_change_bundle_location_cancel" project="catalyst" translatable="false">Cancel</string>
+  <string name="catalyst_change_bundle_location_invalid" project="catalyst" translatable="false">Invalid bundler address: %1$s</string>
   <string name="catalyst_open_debugger_error" project="catalyst" translatable="false">Failed to open DevTools. Please check that the dev server is running and reload the app.</string>
   <string name="catalyst_debug_open" project="catalyst" translatable="false">Open DevTools</string>
   <string name="catalyst_performance_background" project="catalyst" translatable="false">Finish performance trace</string>