Skip to content

Commit df64993

Browse files
vonovakclaude
andcommitted
Fix stale packager connections on Android
DevServerHelper opened and closed the packager connection in separate AsyncTasks on THREAD_POOL_EXECUTOR. A close could run before a pending open, and repeated opens could create two clients. Changing the bundle location from the dev menu also kept the old connection. In both cases, the old dev server could still reload the app. - Run openPackagerConnection and closePackagerConnection as tasks on one single-threaded executor. Tasks run in call order, off the UI thread, and only that thread uses packagerClient. The executor names its thread ReactPackagerConnection and stops it after 30 seconds without work. - Remember the host of the open client. If an open finds a client for a different host, close it and connect to the new host. - Check clientId on the caller's thread. A failed init() still crashes the app, as before; packagerClient stays null, so a later open can retry. - Change Bundle Location: close the packager connection before the reload, as setBundleSource does. Trim the input, then reject a host that OkHttp cannot parse in the connection URL and show a toast. Before, an invalid host was saved without a check. - ReconnectingWebSocket: change close state under the socket's monitor, so no message handler starts after closeQuietly() returns. Close a socket whose handshake finishes after closeQuietly() instead of reporting it as connected. - Add DevServerHelperTest, ReconnectingWebSocketTest and DevSupportHttpClientTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 8d658f4 commit df64993

8 files changed

Lines changed: 425 additions & 62 deletions

File tree

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

Lines changed: 66 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import android.content.Context
1616
import android.net.Uri
1717
import android.os.AsyncTask
1818
import android.provider.Settings.Secure
19+
import androidx.annotation.VisibleForTesting
1920
import com.facebook.common.logging.FLog
2021
import com.facebook.react.bridge.ReactContext
2122
import com.facebook.react.common.ReactConstants
@@ -39,6 +40,10 @@ import java.io.UnsupportedEncodingException
3940
import java.security.MessageDigest
4041
import java.security.NoSuchAlgorithmException
4142
import java.util.Locale
43+
import java.util.concurrent.Executor
44+
import java.util.concurrent.LinkedBlockingQueue
45+
import java.util.concurrent.ThreadPoolExecutor
46+
import java.util.concurrent.TimeUnit
4247
import okhttp3.Call
4348
import okhttp3.Callback
4449
import okhttp3.OkHttpClient
@@ -73,7 +78,8 @@ public open class DevServerHelper(
7378

7479
public fun onPackagerDevMenuCommand()
7580

76-
// Allow apps to provide listeners for custom packager commands.
81+
// Allow apps to provide listeners for custom packager commands. Handlers must not block,
82+
// because closing the connection waits for a running handler.
7783
public fun customCommandHandlers(): Map<String, RequestHandler>?
7884
}
7985

@@ -87,7 +93,14 @@ public open class DevServerHelper(
8793
private val packagerStatusCheck: PackagerStatusCheck = PackagerStatusCheck(client)
8894
private val packageName: String = applicationContext.packageName
8995

96+
// Must stay single-threaded: open and close have to run in call order.
97+
@VisibleForTesting
98+
internal var packagerConnectionExecutor: Executor =
99+
ThreadPoolExecutor(0, 1, 30, TimeUnit.SECONDS, LinkedBlockingQueue()) { runnable ->
100+
Thread(runnable, PACKAGER_CONNECTION_THREAD_NAME)
101+
}
90102
private var packagerClient: JSPackagerClient? = null
103+
private var packagerClientHost: String? = null
91104
private var inspectorPackagerConnection: IInspectorPackagerConnection? = null
92105

93106
/** Returns an opaque ID which is stable for the current combination of device and app, stable */
@@ -138,65 +151,62 @@ public open class DevServerHelper(
138151
get() = settings.isJSMinifyEnabled
139152

140153
public fun openPackagerConnection(clientId: String?, commandListener: PackagerCommandListener) {
141-
if (packagerClient != null) {
142-
FLog.w(ReactConstants.TAG, "Packager connection already open, nooping.")
143-
return
144-
}
145-
object : AsyncTask<Void, Void, Void>() {
146-
@Deprecated("This needs to be rewritten to not use AsyncTasks")
147-
override fun doInBackground(vararg backgroundParams: Void): Void? {
148-
val handlers: MutableMap<String, RequestHandler> = mutableMapOf()
149-
handlers["reload"] =
150-
object : NotificationOnlyHandler() {
151-
override fun onNotification(params: Any?) {
152-
commandListener.onPackagerReloadCommand()
153-
}
154-
}
155-
handlers["devMenu"] =
156-
object : NotificationOnlyHandler() {
157-
override fun onNotification(params: Any?) {
158-
commandListener.onPackagerDevMenuCommand()
159-
}
160-
}
161-
commandListener.customCommandHandlers()?.let { handlers.putAll(it) }
162-
163-
val onPackagerConnectedCallback: ReconnectingWebSocket.ConnectionCallback =
164-
object : ReconnectingWebSocket.ConnectionCallback {
165-
override fun onConnected() {
166-
commandListener.onPackagerConnected()
167-
}
168-
169-
override fun onDisconnected() {
170-
commandListener.onPackagerDisconnected()
171-
}
172-
}
173-
174-
checkNotNull(clientId)
175-
packagerClient =
176-
JSPackagerClient(
177-
clientId,
178-
packagerConnectionSettings,
179-
handlers,
180-
onPackagerConnectedCallback,
181-
)
182-
.apply { init() }
154+
val id = checkNotNull(clientId)
155+
packagerConnectionExecutor.execute {
156+
val host = packagerConnectionSettings.debugServerHost
157+
packagerClient?.let { client ->
158+
if (host == packagerClientHost) {
159+
FLog.w(ReactConstants.TAG, "Packager connection already open, nooping.")
160+
return@execute
161+
}
162+
// The dev server host changed without a close.
163+
client.close()
164+
packagerClient = null
165+
}
166+
val handlers: MutableMap<String, RequestHandler> = mutableMapOf()
167+
handlers["reload"] =
168+
object : NotificationOnlyHandler() {
169+
override fun onNotification(params: Any?) {
170+
commandListener.onPackagerReloadCommand()
171+
}
172+
}
173+
handlers["devMenu"] =
174+
object : NotificationOnlyHandler() {
175+
override fun onNotification(params: Any?) {
176+
commandListener.onPackagerDevMenuCommand()
177+
}
178+
}
179+
commandListener.customCommandHandlers()?.let { handlers.putAll(it) }
183180

184-
return null
181+
val onPackagerConnectedCallback: ReconnectingWebSocket.ConnectionCallback =
182+
object : ReconnectingWebSocket.ConnectionCallback {
183+
override fun onConnected() {
184+
commandListener.onPackagerConnected()
185+
}
186+
187+
override fun onDisconnected() {
188+
commandListener.onPackagerDisconnected()
189+
}
185190
}
186-
}
187-
.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR)
191+
192+
packagerClient =
193+
JSPackagerClient(
194+
id,
195+
packagerConnectionSettings,
196+
handlers,
197+
onPackagerConnectedCallback,
198+
)
199+
.apply { init() }
200+
packagerClientHost = host
201+
}
188202
}
189203

190204
public fun closePackagerConnection() {
191-
object : AsyncTask<Void, Void, Void>() {
192-
@Deprecated("This class needs to be rewritten to don't use AsyncTasks")
193-
override fun doInBackground(vararg params: Void): Void? {
194-
packagerClient?.close()
195-
packagerClient = null
196-
return null
197-
}
198-
}
199-
.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR)
205+
packagerConnectionExecutor.execute {
206+
packagerClient?.close()
207+
packagerClient = null
208+
packagerClientHost = null
209+
}
200210
}
201211

202212
public fun openInspectorConnection() {
@@ -385,6 +395,7 @@ public open class DevServerHelper(
385395

386396
private companion object {
387397
private const val DEBUGGER_MSG_DISABLE = "{ \"id\":1,\"method\":\"Debugger.disable\" }"
398+
private const val PACKAGER_CONNECTION_THREAD_NAME = "ReactPackagerConnection"
388399

389400
private fun getSHA256(string: String): String {
390401
val digest =

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ import com.facebook.react.devsupport.DevServerHelper.PackagerCommandListener
5252
import com.facebook.react.devsupport.InspectorFlags.getFuseboxEnabled
5353
import com.facebook.react.devsupport.StackTraceHelper.convertJavaStackTrace
5454
import com.facebook.react.devsupport.StackTraceHelper.convertJsStackTrace
55+
import com.facebook.react.devsupport.inspector.DevSupportHttpClient
5556
import com.facebook.react.devsupport.inspector.TracingState
5657
import com.facebook.react.devsupport.inspector.TracingStateProvider
5758
import com.facebook.react.devsupport.interfaces.BundleLoadCallback
@@ -450,9 +451,24 @@ public abstract class DevSupportManagerBase(
450451
return@DevOptionHandler
451452
}
452453

453-
ChangeBundleLocationDialog.show(context, devSettings) { host: String ->
454-
devSettings.packagerConnectionSettings.debugServerHost = host
455-
handleReloadJS()
454+
ChangeBundleLocationDialog.show(context, devSettings) { input: String ->
455+
val host = input.trim()
456+
// An empty host resets to the default. An invalid host would crash the connection.
457+
if (host.isEmpty() || DevSupportHttpClient.isValidHost(host)) {
458+
devSettings.packagerConnectionSettings.debugServerHost = host
459+
devServerHelper.closePackagerConnection()
460+
handleReloadJS()
461+
} else {
462+
Toast.makeText(
463+
applicationContext,
464+
applicationContext.getString(
465+
R.string.catalyst_change_bundle_location_invalid,
466+
host,
467+
),
468+
Toast.LENGTH_LONG,
469+
)
470+
.show()
471+
}
456472
}
457473
}
458474

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import com.facebook.react.modules.network.OkHttpClientProvider
1313
import java.util.concurrent.TimeUnit
1414
import okhttp3.ConnectionPool
1515
import okhttp3.Dispatcher
16+
import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
1617
import okhttp3.OkHttpClient
1718

1819
/**
@@ -68,4 +69,10 @@ internal object DevSupportHttpClient {
6869
* the host specifies port 443 explicitly (e.g. "example.com:443").
6970
*/
7071
internal fun wsScheme(host: String): String = if (host.endsWith(":443")) "wss" else "ws"
72+
73+
/**
74+
* Returns whether OkHttp can build a request for the given host, for example "localhost:8081".
75+
*/
76+
internal fun isValidHost(host: String): Boolean =
77+
"${httpScheme(host)}://$host/".toHttpUrlOrNull() != null
7178
}

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,16 +77,18 @@ public class ReconnectingWebSocket(
7777
}
7878

7979
public fun closeQuietly() {
80-
closed = true
81-
closeWebSocketQuietly()
82-
messageCallback = null
80+
synchronized(this) {
81+
closed = true
82+
closeWebSocketQuietly()
83+
messageCallback = null
84+
}
8385

8486
connectionCallback?.onDisconnected()
8587
}
8688

8789
private fun closeWebSocketQuietly() {
8890
try {
89-
webSocket?.close(1_000, "End of session")
91+
webSocket?.close(CLOSE_NORMAL, CLOSE_REASON)
9092
} catch (e: Exception) {
9193
// swallow, no need to handle it here
9294
}
@@ -100,6 +102,10 @@ public class ReconnectingWebSocket(
100102

101103
@Synchronized
102104
override fun onOpen(webSocket: WebSocket, response: Response) {
105+
if (closed) {
106+
webSocket.close(CLOSE_NORMAL, CLOSE_REASON)
107+
return
108+
}
103109
this.webSocket = webSocket
104110
suppressConnectionErrors = false
105111

@@ -155,5 +161,7 @@ public class ReconnectingWebSocket(
155161
private val TAG: String = ReconnectingWebSocket::class.java.simpleName
156162

157163
private const val RECONNECT_DELAY_MS = 2_000L
164+
private const val CLOSE_NORMAL = 1_000
165+
private const val CLOSE_REASON = "End of session"
158166
}
159167
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
<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>
99
<string name="catalyst_change_bundle_location_apply" project="catalyst" translatable="false">Apply Changes</string>
1010
<string name="catalyst_change_bundle_location_cancel" project="catalyst" translatable="false">Cancel</string>
11+
<string name="catalyst_change_bundle_location_invalid" project="catalyst" translatable="false">Invalid bundler address: %1$s</string>
1112
<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>
1213
<string name="catalyst_debug_open" project="catalyst" translatable="false">Open DevTools</string>
1314
<string name="catalyst_performance_background" project="catalyst" translatable="false">Finish performance trace</string>

0 commit comments

Comments
 (0)