[debug] Lazily update frames of all threads in all-stop mode

[sfrylmark]

What it does

A re-make of d1678ad.

The changed bits:

@@ -181,7 +196,17 @@ export class DebugThread extends DebugThreadData implements TreeElement {
         return [...result.values()];
     }
     protected clearFrames(): void {
+        // Clear all frames
         this._frames.clear();
+
+        // Cancel all request promises
+        this.pendingFetchCancel.cancel();
+        this.pendingFetchCancel = new CancellationTokenSource();
+
+        // Empty all current requests
+        this.pendingFetch = Promise.resolve([]);
+        this._pendingFetchCount = 0;
+
         this.updateCurrentFrame();
     }
     protected updateCurrentFrame(): void {

So what's the difference? Well, if you got multiple stops fast enough,
pending frames would be blocking the refresh from happening. Since
updateFrames was being called from one more place (when switching
thread), it forced me to avoid pointless lookups as to not error (GDB
complains when you're trying to fetch something that has already been
fetched). This new behavior that avoids pointless refreshes didn't
take speed into account, and could potentially block an entirely new
fetch.

To fix this, I cancel all frame fetches when clearing frames (which is
done when updating a thread in all-stop mode).

How to test

Same as in #6869, but you should also add vscode-python and try debugging a simple python file, to make sure that isn't broken (see 7264). Debugging NodeJS could also be a good idea.

For what it's worth, I apologize for causing a regression.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[marcdumais-work]

cc: @tom-shan @akosyakov

[akosyakov]

Why do we need to update frames whenever current thread is changed?

[sfrylmark]

Like the comment says,

If this thread is missing stack frame information, then load that.

On a breakpoint or other stop, we clear all threads' frames and only load the current thread's stackframes. When you switch thread it's going to be empty. Which is good, because fetching all stackframes on stop would be slow on a large number of threads.

[akosyakov]

I meant that we should update only according to stop logic, if we do it propertly then by the time when someone changes a thread frames will be already there or at least updating. This code should not be necessary.

[marcdumais-work]

I plant test this PR by debugging in a few languages: Java, Python, go, cpp

[akosyakov]

@marcdumais-work I'm not sure that the original PR was proper. I looked at VS Code logic and they have different logic and timing at least in 2 places. I think the issue is in how we treat stoppedDetails of stop event. Right now we clean all threads and fetch for the current. Instead we should clear all only if allThreadsStopped is present or for a specific thread matching by stoppedDetails.threadId. After update of threads is finished we should refetch call stack for an affected thread not current by using stoppedDetails.threadId. Applying proper semantic of stop event should fix the issue without touching other code.

[sfrylmark]

@akosyakov Actually, we only update in case of all stop mode. See doUpdateThreads and thread.update. I agree it's hard to discover though, I didn't find out about it until you guys commented that there already was stack frame clearing logic.

[akosyakov]

@efrlsml thread.update is called outside of if, so we update always when stoppedDetails are provided?

[sfrylmark]

@akosyakov You're right, unless one remembers to clear stoppedDetails, this might spuriously update. This thing probably needs a refactor (such as taking stoppedDetails as an argument to the update function?), but I'm guessing as you guys like to keep patches small that's outside of the scope for this PR?

[akosyakov]

such as taking stoppedDetails as an argument to the update function?

Is not it already a case?

theia/packages/debug/src/browser/model/debug-thread.tsx

Line 70 in df21bd8

if ('stoppedDetails' in data) {

I'm guessing as you guys like to keep patches small that's outside of the scope for this PR?

I don't think a fix requires big changes, but good understanding of how stop event works in the DAP and in VS Code (a reference implementation) would help.

[sfrylmark]

such as taking stoppedDetails as an argument to the update function?

Is not it already a case?

Nono, I mean literally taking it as a function argument, not assigning to a class-wide variable before calling.

[sfrylmark]

VSCode implementation: https://github.com/microsoft/vscode/blob/b62045619a23f134a515b72294b2eb98ca5f1c74/src/vs/workbench/contrib/debug/browser/debugSession.ts#L728-L759

This calls fetchThreads which does a rawUpdate here. Unlike Theia, VSCode lets the session handle the thread updating (like my first PR originally suggested) https://github.com/microsoft/vscode/blob/b62045619a23f134a515b72294b2eb98ca5f1c74/src/vs/workbench/contrib/debug/browser/debugSession.ts#L655-L674.

Does this answer any questions or provoke any feedback to how our implementation should look? I'm not familiar enough with how Theia is structured to claim that one way is better than the alternative.

[akosyakov]

I don't see any difference in our implementation for update, i.e.

• we compute stoppedDetails in the similar way for each thread here:

  theia/packages/debug/src/browser/debug-session.tsx

  Lines 438 to 450 in 07d0133

  	const data: Partial<Mutable<DebugThreadData>> = { raw };
  	if (stoppedDetails) {
  	if (stoppedDetails.threadId === id) {
  	data.stoppedDetails = stoppedDetails;
  	} else if (stoppedDetails.allThreadsStopped) {
  	data.stoppedDetails = {
  	// When a debug adapter notifies us that all threads are stopped,
  	// we do not know why the others are stopped, so we should default
  	// to something generic.
  	reason: '',
  	};
  	}
  	}

• and then we clean call stack if it was computed

Do you see difference in the logic regardless of structuring?

[sfrylmark]

@akosyakov Yeah, before this patch upstream theia never clears the stack

[akosyakov]

Yeah, before this patch upstream theia never clears the stack

@efrlsml It does on master in DebugThread.update method when stoppedDetails are passed the same way as it is done in VS Code.

The only different which I see that they fetch frames for affected thread, not for the current. Maybe it is a cause of our issues that we are fetching them for not actually stopped thread?

[sfrylmark]

You're right, I forgot master already got the clearing in place (but not the re-freshing). It also already fetches the affected thread, so with this PR merged we are fetching for both the affected thread and the current.

Here seems to be where they fetch the stack frame. Now really, I'm not at all familiar with the VSCode code base so I can't tell you exactly why VSCode works and Theia doesn't. But I can tell you that it does, and I've given you the examples to reproduce this bug in the PR description. I've also made a PR that fixes it, this one.

If we disregard VSCode for a second, this PR intuitively makes sense: In upstream Theia you clear the stack frames for all threads, and you never load them again except for the stopped one. You also say that a thread isn't paused when it has no stack frames. So, all other threads except for the last stopped one appear as "not paused" even though they say STOPPED or PAUSED or something. The single-stepping bug was just a data race where frames were still in the queue and therefore weren't re-fetched. This is now fixed by clearing frames before stepping. Does this make sense?

[akosyakov]

If we disregard VSCode for a second, this PR intuitively makes sense: In upstream Theia you clear the stack frames for all threads, and you never load them again except for the stopped one. You also say that a thread isn't paused when it has no stack frames. So, all other threads except for the last stopped one appear as "not paused" even though they say STOPPED or PAUSED or something. The single-stepping bug was just a data race where frames were still in the queue and therefore weren't re-fetched. This is now fixed by clearing frames before stepping. Does this make sense?

Sounds reasonable. @marcdumais-work Let's merge it and try again?

[marcdumais-work]

Sounds reasonable. @marcdumais-work Let's merge it and try again?

Ok with me but give me an hour or so to at least try it against Python debugging :)

[marcdumais-work]

I tested a debug session with Python using the Microsoft extension v2020.1.58038 and the previous issue (#7264) does not happen any more.

[marcdumais-work]

@marcdumais-work Let's merge it and try again?

Merging.