What does changing this to a Stream do? I think It takes a list so that for -d all the runner can handle multiple vmservice addresses

If I'm reading this correctly, it looks like as a list this has either 0 or 1 elements, so I don't think it's related to -d all. What I'm not sure about is why it had to be an empty list rather than null when there were no observatory uris.

It appears the intention was to support multiple apps running an observatory on a single device, but it's not implemented. Was this done for Fuchsia?

Yeah, it looks like this was for the now-deleted fuchsia_reload command. Fuchsia is now handled differently, so I think this can be safely modified.

At any rate - I'm not really sure what modifying the resident runner here is accomplishing. Isn't the goal to adjust this behavior for attach only?

As per offline discussion, I changed this such that it doesn't reuse the resident runner, which aligns with the expected lifecycle of the resident runner.

If I'm reading this correctly, it looks like as a list this has either 0 or 1 elements, so I don't think it's related to -d all. What I'm not sure about is why it had to be an empty list rather than null when there were no observatory uris.

This seems like a bug. We generally shouldn't be catching Error subclasses unless it is a temporary work around. If you are going to replace uri with uris I would do that in this PR

I'm not sure I follow. If something called cancel too early, then this is the equivalent behavior in the current code.

In what situation does _uriStreamController?.close() triggers the StateError? When you say uris.first after the _uriStreamController?.close()? Can you check _uriStreamController.isClosed or add an onCancel callback to _uriStreamController that records a bit?

The situation is that there's a test returns non-null uri future under protocol_discovery_test.dart that expects uri to return a non-null value. For some reason, this works currently even though the uri future never completes. I checked the documentation for expect and it says:
" If [actual] is a [Future], the test won't complete until the future emits a value." However, it doesn't seem true in this case:

final Completer<void> _completer = Completer<Uri>();
expect(_completer.future, isNotNull);

For this reason, the test is failing when migrated to streams. The stream is closed in the tearDown callback before a value is emitted.

catching a StateError is definitely a bug. It implies there's some logic error in our code.

Here and elsewhere, don't use broadcast streams. These do not buffer events, so if the future resolves before the first subscriber is added the event will be lost

This seems to work fine:

import 'dart:async';

void main() async {
  Stream<String> stream = Stream<String>.fromFuture(Future<String>.value('foo'))
      .asBroadcastStream();

  stream.listen((String value) {
    print(value);
  });
}

prints foo.

The docs for asBroadcastStream() here:

The returned stream will subscribe to this stream when its first subscriber is added, and will stay subscribed until this stream ends, or a callback cancels the subscription.

In general though, I don't know if it would be needed to pass onListen and onCancel callbacks to asBroadcastStream() to pause and resume the underlying stream when the number of subscribers hits zero.

It looks like you re-added the updates to resident runner. Were these changes necessary for some reason?

yes -- observatoryUris is a stream instead of a list.

@blasten If you're changing this API's name anyhow, @Hixie has pointed out that there is no such thing as a URI, they're all URLs, so maybe this should be called urls instead?

That makes sense. Thanks!

Sorry for not catching this earlier, but this fallback will no longer work since the Stream.fromFuture isn't going to return null. That is the null check below is no longer testing the success of the mdns lookup. Could you please do a small follow-up change that checks the return of MDnsObservatoryDiscovery.instance.getObservatoryUri() for null, and if it is null then uses ProtocolDiscovery.observatory() instead? Thanks!

Sounds good. I will follow up on that!