Support reconnects on page refreshes for SSE client connections

[iinozemtsev]

1. reuse reconnection logic between websockets and sse connections
2. persist app instance id on the client side

If we treat reloads as parts of an app lifecycle, it seems natural to preserve an app instance id on full reloads, and looks like a session storage is a good fit.

Fixes #2726.

• Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.

[jyameo]

This looks good to me and addresses the breakpoint flakiness. My main concern is ensuring this doesn't regress multi-browser support or standard stability. Please manually verify that page refreshes, hot restarts, and hot reloads all continue to work as expected. In particular, make sure we can open the app in multiple browsers simultaneously and perform hot reloads on all of them without any interference or connection collisions. Since we lack automated tests for the multi-browser scenario, this manual validation is essential.

Thanks @iinozemtsev

[iinozemtsev]

Thanks @jyameo! I did an excessive internal testing, but I don't have much experience with using fully open-source webdev. I'll try to follow https://github.com/dart-lang/webdev/blob/main/example/README.md tomorrow, but if you have some other app I can try or any other instructions, please let me know.

[srujzs]

This generally LGTM (nice use of sessionStorage).

I'll try to follow https://github.com/dart-lang/webdev/blob/main/example/README.md tomorrow, but if you have some other app I can try or any other instructions, please let me know.

I think those instructions are pretty outdated and I don't know if they still work. I'd try it with Flutter where Flutter tools' pubspec points to your local webdev. flutter run -d chrome should use the ChromeProxyService and flutter run -d web-server should use the WebSocketProxyService. We have testing for hot reloads and restarts in Flutter, but I don't think we have page refresh tests (we might have some in webdev though). And we definitely don't have a test anywhere for the multi-browser case unfortunately.

[iinozemtsev]

It took me a while to figure out how to use flutter tools with local webdev changes, but eventually I succeeded (but unfortunately I left all local changes on my office laptop, so not citing it here).

I found out that e2d14f1 breaks flutter run -d chrome: the app just never loads (cc @Markzipan), so I didn't test this PR yet.

Instead I went down the rabbit hole trying to understand why I can't reproduce #2726 with an external stack.

Turned out that the client -> dwds connection protocol is controlled by a flag and is independent from whether dwds uses chrome proxy service or websocket proxy service:

webdev/dwds/lib/src/config/tool_configuration.dart

Line 99 in c820e2a

this.useSseForInjectedClient = true,

So there were two important differences:

• external flutter web stack communicates with a localhost over websockets, and dwds detects page refreshes instantly when the websocket connection is broken.
• internal Dart web stack communicates through a proxy over SSE, so it leaks connections.

When I tried an internal stack over localhost via chrome remote desktop, it could also reconnect successfully as is, because the SSE connection was instantly switching to keepAliveMode.

I tried enabling websockets internally, and so far it seems to work, but I didn't test how persistent the connection is (e.g. what happens if you leave the debugger unattended for lunch) and it still requires a smaller change to client.dart, I'll send a separate PR for it:

@@ -323,6 +324,10 @@ String _fixProtocol(String url) {
       uri.scheme == 'ws' &&
       uri.host != 'localhost') {
     uri = uri.replace(scheme: 'wss');
+  } else if (window.location.protocol == 'https:' &&
+      uri.scheme == 'ws' &&
+      uri.host != 'localhost') {
+    uri = uri.replace(scheme: 'wss');
   }
   return uri.toString();
 }

I think it still makes sense to land this change, but it also makes sense to try switching the internal stack to websockets.

[iinozemtsev]

Update: yeah, while I've been writing the above comment, the websocket connection got terminated by a timeout after 4 minutes

18:07:01.141 INFO: DwdsInjector: Received request for entrypoint at http://... <-- app connected
18:11:01.612 INFO: DevHandler: injectedConnection.sink.done  <-- proxy killed the websocket
18:33:07.280 SEVERE: DevHandler: Encountered error handling DevTools request. <-- I tried to attach to the app with a debugger
18:33:07.280 SHOUT: ExtensionDebugger: Closing extension debugger due to error. Restart app for debugging functionalityBad state: Failed to start extension debug service. Not connected to an app with id: beklMBgPNDr4uTm2FjAGMQ==

Upd: leaving the setup mid-debug (paused on a breakpoint) for 4+ minutes and ten trying to restart from an ide just crashes the whole DDR :)

18:52:34.410 INFO: DwdsVmClient: Attempting to disable breakpoints and resume the isolate
Uncaught error, shutting down: Bad state: No active isolate to resume.
package:dwds/src/dwds_vm_client.dart 565:7                    ChromeDwdsVmClient._disableBreakpointsAndResume

What do you think about sending empty heartbeat messages from the client by a timer?

[iinozemtsev]

last update for today: looks like neither server, nor client heartbeats go through when the app is paused in the debugger and left unattended, so I think the internal setup should stick to SSE.

It also means that this change is unlikely to affect other stacks, which use websockets: for websocket proxy service this change is a mere refactor, it changes only sse logic. We can make it even "less affecting" by scoping the client.dart change (ids from session storage) only for SSE connections, but that's probably too defensive.

[srujzs]

I found out that e2d14f1 breaks flutter run -d chrome: the app just never loads (cc @Markzipan), so I didn't test this PR yet.

Thanks for bringing this up. I have a fix for this: flutter/flutter#180108

I think it still makes sense to land this change, but it also makes sense to try switching the internal stack to websockets.

Having consistent code everywhere sounds great, but per your investigation, it looks like that's not tractable in the near-term so this is fine especially since that's the current state.

We can make it even "less affecting" by scoping the client.dart change (ids from session storage) only for SSE connections, but that's probably too defensive.

I have a feeling this would complicate the code (both in client.dart and elsewhere) so I agree.