Repository navigation
"flutter attach --machine" provides a malformed VM Service URI #118609
Description
Activity
If you pass a
ws://.../wsfor--debug-uriyou get the same behaviour. Though if you pass--no-ddsyou get slightly different results (correct results when passing a http root, but incorrect when passing the ws address):// args --debug-url ws://127.0.0.1:51458/Xxj5fo0dHj0=/ws --no-dds // wsUri ws://127.0.0.1:51458/Xxj5fo0dHj0=/ws/ws// args --debug-url http://127.0.0.1:51458/Xxj5fo0dHj0= --no-dds // wsUri ws://127.0.0.1:51458/Xxj5fo0dHj0=/ws@christopherfujino @bkonyi before I start trying to fix anything here, can we confirm what formats of URI should be supported here? I notice that a lot of the integration tests re only using port numbers (and
--disable-auth-codes) which means they're not seeing any of this behaviour. I've started changing the tests to leave auth-codes enabled and just handle proper URIs instead of only port numbers for better coverage, but I want to be sure I'm making the right assumptions about what we need to pass for--debug-uri(I'm assuming that the outputwsUrishould always startws:/and end/ws).@christopherfujino @bkonyi before I start trying to fix anything here, can we confirm what formats of URI should be supported here? I notice that a lot of the integration tests re only using port numbers (and
--disable-auth-codes) which means they're not seeing any of this behaviour. I've started changing the tests to leave auth-codes enabled and just handle proper URIs instead of only port numbers for better coverage, but I want to be sure I'm making the right assumptions about what we need to pass for--debug-uri(I'm assuming that the outputwsUrishould always startws:/and end/ws).Sorry, I can't be of much help here, I don't know how any of this works. Also, is this a recent regression? This seems like a pretty bad breakage.
This is not a recent regression.
It's happening because flutter_tools is trying to recover a DDS URI by parsing an error message returned in an exception (see https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/base/dds.dart#L74)
The exception message adds a period after the URI (see https://github.com/dart-lang/sdk/blob/main/sdk/lib/vmservice/vmservice.dart#L274). The message is also formatted in a JSON-like style with comma-delimited fields.
So the parser captures the
.,suffix as part of the URI.Ah, thanks! It looks like we should probably add a field to the exception (or a subclass of this exception) for the URI and then it won't need to be extracted from the message like this. I'll take a look, thanks!
@bkonyi I now realise the code linked above is part of
lib/vmservice/vmservice.dartand not inside DDS (and that the error is going over the JSON protocol) so it's not as simple as just changing the exception and adding a field.Although, I'm curious why we need to extract the URL here. If there's already a DDS instance can't we use the original VM Service URL? It seems like we're doing that in DAP - we're handling the same exception, but just using the same URL:
I'm not sure if the is a difference in requirements here, or whether maybe DAP isn't doing the correct thing?
If the VM service client is capable of handling redirects, using the VM service URI instead of the DDS URI works fine since the VM service will redirect requests from non-DDS clients to DDS.
However, if a web client like DevTools tries to make a websocket connection to the VM service directly when there's a DDS instance attached, the connection will fail since the
dart:htmlWebSocketimplementation doesn't follow redirects (and there's no way to tell it to follow them). I had originally intended for the DDS URI to be completely transparent to clients, having the VM service redirect requests to DDS, but this restriction made it impossible to rely on redirects which is why we need to output the DDS URI.@bkonyi ah, I see! Is it possible/reasonable to extend the exception in some way to carry this URI over without Flutter needing to parse it? (I couldn't figure out where the error was being deserialised to be used in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/base/dds.dart#L74 to know whether it could have additional field over just
message)Yeah, we probably could do that. The exception is created here: https://github.com/dart-lang/sdk/blob/376886ed1da63f81196c72490830fafd954b87d5/pkg/dds/lib/src/dds_impl.dart#L214.
We don't do any processing of the exception in DDS right now, but it's something we could do.
Cool, lemme take a look. Thanks!
- added a commit that references this issue
on Jan 18, 2023 - added a commit that references this issue
on Jan 30, 2023 I have a fix at #119506. It makes two changes:
- Use a new field on the exception that holds exactly the URI (this requires the existing DDS instance provided it so is not guaranteed since we don't know what version it is if we didn't spawn it)
- If we have to fall back to parsing the exception text, we trim trailing fullstops
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.- locked as resolved and limited conversation to collaborators
on Mar 3, 2023
The
wsUriprinted when usingflutter attach --machineappears to be malformed:The
wsUrireturned isws://127.0.0.1:49606/h-O0daymeDM=/.,/wsbut I would expect it to bews://127.0.0.1:49606/h-O0daymeDM=/ws.