VmService does not fail in-flight requests when the connection closes unless dispose() is explicitly called

[DanTup]

Moving this from discussion at flutter/flutter#147250 as the issue here is in the base debug adapter.

When trying to roll the latest DDS (with DAP) into Flutter, some tests are timing out. This starts happening with 717aa63.

With the changes above, we can all output for stack frames (to linkify them). This may involve async work if we need to resolve package:foo to a file path (done by calling lookupResolvedPackageUris). Output events are queued to ensure output always arrives in the correct order and when we're shutting down, we wait for the output queue to drain (so that you don't miss potentially-important output - such as exception details - if the application terminates).

For reasons I don't understand, the call to lookupResolvedPackageUris never completes in some (most) cases when running those tests. I would have thought it would throw (possibly "The client closed with pending request"?).

The flutter test process exits anyway, and as part of our shutdown we then wait for all output events to drain - but this never happens because they're queued behind an output event that's stalled on lookupResolvedPackageUris.

One possible fix is to say that we never wait for more than x seconds for output events during shutdown. This would ensure we always terminate if the target process is gone, no matter what's happening with output events. However, it would be better if we could ensure lookupResolvedPackageUris just terminated during shutdown instead, because there is error handling that would then allow the output events to still be sent (without any linkifying).

@bkonyi any ideas why the request wouldn't be failing given that the target process has exited? (is it possibly because we're connected to DDS and not to the target process? Does it handle this?)

[DanTup]

One possible fix is to say that we never wait for more than x seconds for output events during shutdown.

Actually, this doesn't fix the Flutter tests, because they're asserting the error output and it doesn't arrive in this case. If we can't have lookupResolvedPackageUris automatically fail, we should look at forcing it to fail (when handleSessionTerminate is triggered).

[bkonyi]

@bkonyi any ideas why the request wouldn't be failing given that the target process has exited? (is it possibly because we're connected to DDS and not to the target process? Does it handle this?)

DDS is setup in such a way that it automatically shuts itself down when the VM service instance it's connected to disappears. If this isn't happening, there would have to be some weird edge case where the VM service connection isn't closed properly and DDS doesn't get a "The client closed with pending requests" exception, or there's something in DDS that's preventing it from shutting down due to the outstanding request.

AFAIK, DDS immediately shuts down its HTTP server once the target VM service disappears which should be enough to kill all the active connections to DDS.

If this is something you can easily reproduce, it might be worth adding some logging to DDS to trace whether or not lookupResolvedPackageUris is actually blocked on a future somewhere. Another option would be to disable serving DDS and rerunning the test to see if the behavior is still reproducible.

[DanTup]

@bkonyi I tested without DDS and the issue still occurred. I was looking at where the outstanding requests are rejected and found that that's done in VmService.dispose(), however I can't find anything that would be triggering that when the connection closes. We do pass a streamClosed future to the VmService constructor (which is completing), but it doesn't seem to trigger that code.

So, I added an explicit call to service.dispose() when the onDone future completes (which is triggered by our streamClosed future) and that fixed the issue:

So I guess my question is whether VmService should be automatically handling outstandingRequests (which is done in dispose()) if the future passed to streamClosed is completed? Or should I be explicitly calling dispose as in the code above?

[DanTup]

Based on discussions with @bkonyi, I believe this will be handled in VmService not not DAP/DDS. I think once fixed there, updating to a new pkg:vm_service with the change will fix flutter/flutter#147250 without any additional changes in pkg:dds.