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

[DanTup]

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:

• Clarify range of number microsoft/debug-adapter-protocol#422
• nvim-dap will JSON encode very large integers in scientific notation mfussenegger/nvim-dap#1004
• Don't use numbers larger than will fit into double-precision floating points in the VM Service for identifiers #53081

[bkonyi]

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]

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

[DanTup]

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]

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]

@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]

Thanks works for me, thank you!

[DanTup]

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

[lrhn]

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]

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]

CP request here