Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to debug any apps using most recent Dart VM builds #2464

Closed
DanTup opened this issue May 16, 2020 · 14 comments
Closed

Unable to debug any apps using most recent Dart VM builds #2464

DanTup opened this issue May 16, 2020 · 14 comments
Labels
important Something important! in debugger Relates to the debug adapter or process of launching a debug session is bug
Milestone

Comments

@DanTup
Copy link
Member

DanTup commented May 16, 2020

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.

@DanTup DanTup added is bug in debugger Relates to the debug adapter or process of launching a debug session important Something important! labels May 16, 2020
@DanTup DanTup added this to the v3.11.1 milestone May 16, 2020
@DanTup
Copy link
Member Author

DanTup commented May 16, 2020

May be more than just adding jsonrpc though:

==> {"jsonrpc":"2.0","id":"0","method":"getVersion"}
<== {"jsonrpc":"2.0","error":{"code":-32602,"message":"Parameters for method \"getVersion\" must be passed by name.","data":{"request":{"jsonrpc":"2.0","id":"0","method":"getVersion"}}},"id":"0"}

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
Copy link
Member Author

DanTup commented May 16, 2020

Seems like params is required:

==> {"jsonrpc":"2.0","id":"1","method":"getVM","params":{}}
<== {"jsonrpc":"2.0","result":{"type":"VM","name":"vm","architectureBits":64,

Easy to fix here, though the jsonrpc spec says params may be omitted, so this might be something the VM should tolerate.

@DanTup DanTup closed this as completed in 2a90dfd May 16, 2020
@devoncarew
Copy link
Contributor

I think the VM should be tolerant to both jsonrpc and params not 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
Copy link
Member Author

DanTup commented 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
Copy link
Contributor

Yeah, that's what I'm worried about, esp. as there's no real value in additional strictness for these items.

@bkonyi
Copy link

bkonyi commented 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
Copy link
Contributor

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.

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
Copy link

bkonyi commented 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 allowMalformed flag for the Server and Peer classes that loosens enforcement of spec requirements for non-essential fields.

@devoncarew
Copy link
Contributor

I'd argue it's probably easier just to fix the clients to send the correct format.

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.

Maybe we could update the package to include an allowMalformed flag for the Server and Peer classes that loosens enforcement of spec requirements for non-essential fields.

That seems pretty feasible.

@bkonyi
Copy link

bkonyi commented May 18, 2020

I'd argue it's probably easier just to fix the clients to send the correct format.

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.

Ugh, right, I forgot the release cycle of some clients is quite long, not even considering the fact that some users never upgrade...

Maybe we could update the package to include an allowMalformed flag for the Server and Peer classes that loosens enforcement of spec requirements for non-essential fields.

That seems pretty feasible.

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
Copy link
Contributor

I don't see much recent work on package:json_rpc_2; perhaps @natebosch could also review PRs there?

@natebosch
Copy link

Yes I can review PRs in that package. To clarify my understanding:

  • We have a server which recently started using package:json_rpc_2.
  • We have clients which are not using that package, and are not strictly following the protocol.
  • We'd like to allow non-strict protocol usage in the server, and to achieve that we'll add a new flag in 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
Copy link

bkonyi commented May 18, 2020

Yes I can review PRs in that package. To clarify my understanding:

  • We have a server which recently started using package:json_rpc_2.

Correct. package:dds depends on package:json_rpc_2 and is now launched as an intermediary between clients and the true VM service.

  • We have clients which are not using that package, and are not strictly following the protocol.

Correct. The VM service has historically been lenient when it comes to requiring the jsonrpc parameter in requests.

  • We'd like to allow non-strict protocol usage in the server, and to achieve that we'll add a new flag in package:json_rpc_2. This will only impact what we accept, and not what we emit. We'd still always emit strictly correct messages.

Yes, that sounds right.

I would be OK with that and happy to review a PR.

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
Copy link

One suggestion on naming before the PR: I'd go with strictProtocolChecks: false as the argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
important Something important! in debugger Relates to the debug adapter or process of launching a debug session is bug
Projects
None yet
Development

No branches or pull requests

4 participants