Do not try to refresh threads to compute resume/suspend

[mickaelistria]

Those operations typically work on the list of known threads, there is no need to refresh the threads at that time, and in case DAP's threads() request doesn't run well on non-suspended targets, then the former implementation could lead to bogus states.

[mickaelistria]

@jonahgraham This has become an issue with threads() not answering (and theads.get() stuck) in vscode-js-debug. I think my proposal makes sense, but I would feel better if you can review it quickly to at least tell me the reasoning doesn't seem too wrong.

[jonahgraham]

I understand the idea you have here, but I think it will lead to some display errors on targets that do support querying threads when running.

I think the correct approach is to fix:

	@Override
	public void thread(ThreadEventArguments args) {
		refreshThreads.set(true);
		fireChangeEvent(DebugEvent.CONTENT);
	}

to update the list of theads based on the information in ThreadEventArguments and not set refreshThreads. The doing the full refresh on every thread event was really a shortcut and not what I assume that VSCode does.

(BTW I reviewed this quite quickly as I need to step away from my desk - I can review and run it up more fully later if the above seems like nonesense or if you are seeing something else)

[mickaelistria]

You're probably right, I will try to implement as you suggest and see if it works.

[mickaelistria]

@jonahgraham This implements your proposal and seems to work fine for vscode-js-debug.

[jonahgraham]

So it turns out this breaks a common use case and makes me realize there was always a bug there.

For single thread DAPs it is common to not send thread events at all as the one thread is created at configurationDone. The pre-existing code would call "threads" at least once.

The VSCode mock debug adapter works like this, and as mock debug is the basis for many authors of debug adapters I expect other adapters to face this issue.

---

The rest is the details about a bug that was always there.

The bug is that the threads call would never be sent after the initial one, unless a thread event was received. This is incorrect as the client must send a threads event after each stopped event received (from https://microsoft.github.io/debug-adapter-protocol/overview):

Supporting threads

Whenever the generic debugger receives a stopped or a thread event, the development tool requests all threads that exist at that point in time. Thread events are optional, but a debug adapter can send them to force the development tool to update the threads UI dynamically even when not in a stopped state. If a debug adapter decides not to emit Thread events, the thread UI in the development tool will only update if a stopped event is received.

After a successful launch or attach, the development tool requests the baseline of currently existing threads with the threads request and then starts to listen for thread events to detect new or terminated threads. Even if a debug adapter does not support multiple threads, it must implement the threads request and return a single (dummy) thread. The thread id must be used in all requests which refer to a thread, e.g. stacktrace, pause, continue, next, stepIn, and stepOut.

Basically the existing code supports two of the three cases enumerated in the above block, those cases being:

1. Single thread (or single synthetic thread)
2. Multi-threads with thread events
3. Multi-threads with no thread events <-- this is the missing one

---

The above is relevant to your case as the adding you made in threads is partially correct, but should not remove the existing code around refreshThreads - but it should not set refreshThreads in the thread event, but should set in the stopped event.

I don't think you should be calling threads in the thread event.

[jonahgraham]

(We need automated tests as that would have immediately identified the probable regression here - I will talk to Renesas to see if I can get some funded time to work on that.)

[mickaelistria]

OK, here is another attempt which I believe matches the spec (query threads on stopped or on threads event)

[jonahgraham]

This looks better, and I have re-read the spec and AFAICT this does implement the spec, but in practice debug adapters don't need to send an initial stopped or thread event to kick things off. VSCode always follows "configurationDone" with a "threads" - which means the debug adapter doesn't need to provide a stopped or thread event to get its initial state correct.

---

I am concerned over the removal of the synchronization in this code. IIUC the update you have here the contents of thenAcceptAsync (in triggerUpdateThreads) may be running in multiple threads simultaneously as triggerUpdateThreads may be called multiple times in a row before the async part of previous calls has finished.

[jonahgraham]

Note that in case where multiple threads are started in quick succession this will cause multiple threads() calls to be made in quick succession.

[mickaelistria]

New patch set re-adds synchronization and implements your 2 suggestions/fixes. For the multiple request to threads() in case multiple notification are sent; I'm not sure what would be the best solution.
Do you think it's something we need to fix right now? If it's acceptable, I can leave a comment about this in the code and we can optimize it later when needed.

[jonahgraham]

Do you think it's something we need to fix right now?

No.

[jonahgraham]

This looks ok to me, just this comment unresolved:

This looks better, and I have re-read the spec and AFAICT this does implement the spec, but in practice debug adapters don't need to send an initial stopped or thread event to kick things off. VSCode always follows "configurationDone" with a "threads" - which means the debug adapter doesn't need to provide a stopped or thread event to get its initial state correct.

The current version won't display threads correctly for such debug adapters, but they will work in VSCode.

[mickaelistria]

OK, I've added an instruction to trigger a threads update upon initialization.

[mickaelistria]

Thank you very much for you review @jonahgraham !