Repository navigation
Unable to debug any apps using most recent Dart VM builds #2464
Description
Using current Flutter master, or Dart nightly build debug sessions don't launch. It looks like we're not passing jsonrpc in the requests (this seems incorrect, but the VM may have previously been accepting it?).
[6:56:40 PM] [Observatory] [Info] [Dart] ==> {"id":"0","method":"getVersion"}
[6:56:40 PM] [Observatory] [Info] [Dart] <== {"jsonrpc":"2.0","error":{"code":-32600,"message":"Request must contain a \"jsonrpc\" key.","data":{"request":{"id":"0","method":"getVersion"}}},"id":"0"}
@devoncarew FYI - I'll work on a fix and make a patch release. Not sure if this will affect IntelliJ or others though.
Activity
DanTup commentedon May 16, 2020
May be more than just adding
jsonrpcthough:I'm not entirely sure what this error means (or where it's coming from - I can't find it in the Dart source).
DanTup commentedon May 16, 2020
Seems like
paramsis required:Easy to fix here, though the jsonrpc spec says
paramsmay be omitted, so this might be something the VM should tolerate.Fix jsonrpc messages for VM service
devoncarew commentedon May 18, 2020
I think the VM should be tolerant to both
jsonrpcandparamsnot existing in the request. Can you open an issue on the SDK repo, and cc @bkonyi ? Not sure if this is related to changes in how the VM service impl. parses things, package:vm_service changes, or recent work to support DDS.DanTup commentedon May 18, 2020
I've opened dart-lang/sdk#41942.
I did push out a patch to Dart-Code on Saturday so as long as people upgrade they shouldn't hit it here, but I don't know if there are other tools that'll be caught by this.
devoncarew commentedon May 18, 2020
Yeah, that's what I'm worried about, esp. as there's no real value in additional strictness for these items.
bkonyi commentedon May 18, 2020
These restrictions are unfortunately enforced by package:json_rpc_2 which we've been using in DDS. This issue was resolved in package:vm_service 4.0.2 so any tools which use the package should already have a fix.
Short of forking package:json_rpc_2 to remove the restrictions or a significant DDS rewrite, I don't think there's much we can do here.
devoncarew commentedon May 18, 2020
I'd consider doing that, having rpc2 not enforce those options, or, just using a simpler package (json rpc is a fairly simple protocol).
bkonyi commentedon May 18, 2020
I'd argue it's probably easier just to fix the clients to send the correct format. DDS uses package:json_rpc_2 quite heavily but maintaining a fork to work around the protocol requirements seems like overkill. Maybe we could update the package to include an
allowMalformedflag for theServerandPeerclasses that loosens enforcement of spec requirements for non-essential fields.devoncarew commentedon May 18, 2020
Likely very difficult given there are existing clients, and the upgrade cycle is several months long. If we go this route, we should revert this, update clients, wait a period of time, and add this change back.
That seems pretty feasible.
bkonyi commentedon May 18, 2020
Ugh, right, I forgot the release cycle of some clients is quite long, not even considering the fact that some users never upgrade...
It's probably the easiest fix here. I don't think we have a real owner of the package, but maybe @kevmoo has an opinion on this?
devoncarew commentedon May 18, 2020
I don't see much recent work on
package:json_rpc_2; perhaps @natebosch could also review PRs there?natebosch commentedon May 18, 2020
Yes I can review PRs in that package. To clarify my understanding:
package:json_rpc_2.package:json_rpc_2. This will only impact what we accept, and not what we emit. We'd still always emit strictly correct messages.I would be OK with that and happy to review a PR.
bkonyi commentedon May 18, 2020
Correct. package:dds depends on
package:json_rpc_2and is now launched as an intermediary between clients and the true VM service.Correct. The VM service has historically been lenient when it comes to requiring the
jsonrpcparameter in requests.Yes, that sounds right.
Excellent! I'll pull something together today and send it your way. Once we publish a new version I'll update the DEPS in the SDK and hopefully get this resolved today.
natebosch commentedon May 18, 2020
One suggestion on naming before the PR: I'd go with
strictProtocolChecks: falseas the argument.