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

DAP threadIds can currently be 53bit integers but the DAP spec specifies 32bit integers #53086

Closed
DanTup opened this issue Aug 1, 2023 · 10 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation pkg-dds For issues related to the Dart Development Service

Comments

@DanTup
Copy link
Collaborator

DanTup commented Aug 1, 2023

We recently started using Isolate.numbers from the VM Service as the threadIds in the debug adapter. It turned out that these numbers were random 64bit numbers and could easily be values that aren't precisely representable in languages that use 64bit floating point numbers.

This was fixed via #53081 by reducing these numbers to 53bits.

However, with some more digging it's not clear that 53bit integers are allowed in DAP either. The spec says:

integers defined in the protocol (JSON schema type integer) may be represented as 32 bit signed integers, although some properties disallow negative values. numbers in the protocol (JSON schema type number) may be represented as 64 bit floating point numbers.

Although the online spec has threadId as number, the JSON spec uses integer.

Assuming the correct type is integer, this means we cannot use > 32bits for threadIds. This would mean either:

  1. The VM Service IDs need reducing further from 53bit to 32bit integers
  2. We should stop using the Isolate.number directly as threadId and instead add a custom API for mapping back and forth

@bkonyi @jacob314 thoughts/opinions?

(cc @elliette @christopherfujino)

Related:

@DanTup DanTup added pkg-dds For issues related to the Dart Development Service dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation labels Aug 1, 2023
@bkonyi
Copy link
Contributor

bkonyi commented Aug 1, 2023

After chatting with @rmacnak-google, I don't think we'll be able to reduce the ID space any further. The isolate ID is really the isolate's main port and is meant to be "unguessable", so reducing the ID space weakens this property (ideally we'd have a 128-bit ID).

If the DAP requires 32-bit thread IDs, we'll need to take a different approach that involves mapping between isolate and thread IDs.

@rmacnak-google
Copy link
Contributor

Right, send ports are capabilities and their IDs should be Swiss numbers.

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 1, 2023

Thanks - we'll look at doing something different then.

@elliette are you already using this? Which functionality is it required for - and is it in both directions or only one?

@elliette
Copy link
Contributor

elliette commented Aug 1, 2023

I am already using this, but it's currently disabled by a feature flag: https://github.com/flutter/devtools/blob/6f42c1d59d95dcacde130f2910c1d10be8508e61/packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart#L616

It's just being used to go from VM Frame to DAP frame.

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 2, 2023

@elliette would it work for you if we include isolateIds in the response to DAP threads? It means you'd need to call DAP for threads but from the response you'd have a mapping of threadId <-> isolateId (which you can cache and just re-call threads if you discover a thread/isolate that's not in your cache).

A change for this is here (the extra fields are only included over DDS to avoid the non-standard field for standard DAP clients):

https://dart-review.googlesource.com/c/sdk/+/317701

Which depends on this change that reverts the original code (which fixes this issue):

https://dart-review.googlesource.com/c/sdk/+/317700

We can land these together and then you could switch over.

@elliette
Copy link
Contributor

elliette commented Aug 2, 2023

Thanks works for me, thank you!

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 2, 2023

Great, thanks for confirming! I've opened reviews for them.

@mraleph mraleph added the area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. label Aug 3, 2023
copybara-service bot pushed a commit that referenced this issue Aug 21, 2023
See #53086

Change-Id: I380744f9e0d604168f026684b31fe689bb8947c8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/317701
Reviewed-by: Ben Konyi <[email protected]>
@lrhn
Copy link
Member

lrhn commented Nov 19, 2023

Assuming the correct type is integer, this means we cannot use > 32bits for threadIds.

For the record, since this issue has become relevant again,
the JSON-schema definition of "integer" is:

integer
    A JSON number without a fraction or exponent part.

That is not limited to 32-bit integers. It is likely effectively limited to 53-bit integers, but anything up to that should work, ad long as it's serialized without any fractions or exponents.

@DanTup
Copy link
Collaborator Author

DanTup commented Nov 19, 2023

The quote above about not using 32bits was because of what's written in the DAP spec - it seems like that might be more restrictive. I think we should stick to > 32bits for DAP integers.

This issue was fixed by going back to increasing integers starting at 1 for threadIds, but it seems the Flutter stable release unfortunately ended up with a DDS release between the break and the revert 😞

@christopherfujino
Copy link
Member

CP request here

auto-submit bot pushed a commit to flutter/flutter that referenced this issue Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation pkg-dds For issues related to the Dart Development Service
Projects
None yet
Development

No branches or pull requests

7 participants