DTDManager handles the subscription to the service stream #9635
Conversation
| if (_connection.value case final connection?) { | ||
| await connection.close(); | ||
| } | ||
| _connection.value = null; |
There was a problem hiding this comment.
I noticed that we were setting the connection to null without closing it first
| _connection.value = connection; | ||
| // Close the previous connection. | ||
| if (previousConnection != null) { | ||
| await previousConnection.close(); |
There was a problem hiding this comment.
Before this change, we were closing the connection during the disconnect implementation. I moved it here to the connect implementation instead so that we wait for the new connection before closing the previous connection (this prevents us from dropping any service extension registration events we receive while reconnecting).
There was a problem hiding this comment.
nit: await previousConnection?.close() would remove the if :)
(is it possible it could be triggered twice? would that cause an error? If so, maybe it should be set to null afterwards too?)
elliette
requested review from
a team,
DanTup,
bkonyi and
srawlins
and removed request for
a team,
bkonyi and
kenzieschmoll
| expect(await eventQueue1.next, equals(fooBarRegisteredEvent)); | ||
| expect(await eventQueue2.next, equals(fooBarRegisteredEvent)); | ||
|
|
||
| await eventQueue1.cancel(); |
There was a problem hiding this comment.
In case the above expectations fail, should all of this be in a try/finally, to not hang the process or screw up the following tests?
try {
expect(...);
} finally {
await eventQueue1.cancel();
await eventQueue2.cancel();
}| await manager.connect(fakeDtdUri); | ||
|
|
||
| // Subscribe to the service registration stream. | ||
| final eventQueue = StreamQueue( |
There was a problem hiding this comment.
I'm not sure that this test ensures that invalidEvent is not sent. But I think we can simplify it with the stream matchers in package:test:
expect(manager.serviceRegistrationBroadcastStream,
emitsInOrder([
fooBarRegisteredEvent,
fooBarUnregisteredEvent,
emitsDone,
]),
);
DanTup
left a comment
DanTup
left a comment
There was a problem hiding this comment.
FYI @DanTup I've changed the DTD reconnection logic a little bit. I tested the Flutter panel and Property Editor in VS Code, but you might want to verify as well.
LGTM - I'll make a note to have a quick test next week too. Thanks!
| /// A stream of [CoreDtdServiceConstants.serviceRegisteredKind] and | ||
| /// [CoreDtdServiceConstants.serviceUnregisteredKind] events. | ||
| /// | ||
| /// Since this is a broadcast stream, it supports multiple subscribers. |
There was a problem hiding this comment.
Perhaps it's worth a note/hint on this comment suggesting that if listening to this, callers likely also want to call getRegisteredServices after subscribing to ensure any already-registered services are considered. It might be easy to forget that (given the original API was a single-listener stream that would've buffered) and introduce races.
| _connection.value = connection; | ||
| // Close the previous connection. | ||
| if (previousConnection != null) { | ||
| await previousConnection.close(); |
There was a problem hiding this comment.
nit: await previousConnection?.close() would remove the if :)
(is it possible it could be triggered twice? would that cause an error? If so, maybe it should be set to null afterwards too?)
3 participants
Fixes #9632
DTDManagernow subscribes to theServicestream, and broadcastsServiceRegisteredandServiceUnregisteredevents to any subscribers.Currently the
EditorClientis the only subscriber, but our AI Assistant will also need to subscribe to those events.FYI @DanTup I've changed the DTD reconnection logic a little bit. I tested the Flutter panel and Property Editor in VS Code, but you might want to verify as well.