[ DWDS ] Disconnect non-DDS clients when DDS connects

[bkonyi]

The native VM service disconnects all non-DDS clients when DDS connects in order to keep state consistent. DWDS was never configured to work this way and, instead, would not let DDS connect if there were already existing clients.

This is fine most of the time, as DDS is typically the first client, but in cases where multiple DDS instances attempt to start at once (e.g., a simultaneous flutter run and flutter attach), this inconsistency prevents tools from falling back to using an already connected DDS instance, as seen in flutter/flutter#171758.

Fixes #2399

[github-actions]

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:dwds	25.0.2	ready to publish	dwds-v25.0.2
package:frontend_server_client	4.0.0	already published at pub.dev
package:webdev	3.8.0-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

[nshahan]

Sorry I don't understand what the context is here. Can you elaborate on what the other connected clients can be? What would the end user typically be doing to notice some client getting disconnected because a dds client connected? Do they have a path to recovery in that situation?

[bkonyi]

Sorry I don't understand what the context is here. Can you elaborate on what the other connected clients can be? What would the end user typically be doing to notice some client getting disconnected because a dds client connected? Do they have a path to recovery in that situation?

Not a problem, this is a bit of a niche DDS implementation detail.

Basically, DDS presents itself as the VM service to clients, even though it's actually middleware. Since clients are connecting to DDS and not the VM service directly, DDS needs to be able to handle all of the VM service functionality that is client dependent, including sending service events to clients for streams they've listened to and service extension routing.

If DDS is connected to a VM service and there's also other clients connected directly to the VM service, things can get into an inconsistent state. For example, the VM service itself won't know about service extensions registered by clients with DDS, so any clients connected directly to the VM service won't be able to invoke those service extensions. To prevent this, the VM service implementations have a special, private _yieldControlToDDS RPC that's invoked by DDS to put the VM service into "single client mode". This is meant to ensure that DDS is the only client of the VM service.

The native VM service has always disconnected non-DDS clients when this RPC is invoked. Before disconnecting a client, it first sends an event on the Service stream notifying them that DDS has connected at a given URI and that they're going to be disconnected. Clients can handle this event to re-establish their VM service connection via DDS (although I'm not sure this is actually done by any known client).

DWDS never actually implemented this behavior, and instead opted to throw an exception when _yieldControlToDDS was invoked if there was already another direct connection to DWDS. In most situations, that's fine. However, if there's two tools that try to start DDS at the same time, _yieldControlToDDS should be throwing an ExistingDartDevelopmentServiceException to the DDS instance that tries to start second. These tools should be configured to detect this failure mode and fallback to connecting directly to the DDS instance that connected successfully.

This situation used to never happen. However, Dart-Code does have some scenarios where it runs flutter attach to wait for a VM service to become available while also running flutter run to start the application. flutter attach has logic to try to start DDS if it doesn't detect an existing instance, and flutter run always tries to start DDS itself, which causes a race where one tool will fail to start DDS and should connect to the URI in the resulting ExistingDartDevelopmentServiceException. Before this PR, DWDS was throwing an exception that DDS didn't recognize and would cause DDS to basically throw an internal error that the tool couldn't recover from. This PR makes DWDS's behavior consistent with how the native VM service handles this scenario.

[srujzs]

Thanks for the comment! I...sort of understand what the objective is. :) DDS could never throw a ExistingDartDevelopmentServiceException correctly because DWDS would throw some other internal exception that DDS didn't know how to handle, is that right?

[nshahan]

Thanks for the context writeup. LGTM.

[srujzs]

I'm trying to land a bug fix as well here btw: #2673 and would like to put both these in the same bug fix version. Should I land that first or after this?

[bkonyi]

Thanks for the comment! I...sort of understand what the objective is. :) DDS could never throw a ExistingDartDevelopmentServiceException correctly because DWDS would throw some other internal exception that DDS didn't know how to handle, is that right?

That's correct. DDS was seeing an ExistingDartDevelopmentServiceException that didn't include the URI for the existing DDS instance, so it just threw an error that wouldn't allow for tooling to just connect to the existing instance.

[bkonyi]

I'm trying to land a bug fix as well here btw: #2673 and would like to put both these in the same bug fix version. Should I land that first or after this?

I'd like to get this change landed ASAP as it's one of our top crashers in the Flutter tool, so if I could land first I'd appreciate it :)

Are you planning on cherry picking that change into Flutter by any chance? I'll need to create a hotfix release on top of 24.4.0 with this change to CP it into Flutter stable. If you need your change included, I can be sure to include it in my hotfix release.

[srujzs]

Thanks!

so if I could land first I'd appreciate it :)

Go for it! I'll land a separate bug fix version once you publish this one and update Flutter to use that.

Are you planning on cherry picking that change into Flutter by any chance?

No, this is purely just to get web socket hot restart working in Flutter, which isn't enabled in stable anyways.