vscode: Breakpoints are unexpectedly shown as not-verified after removing and re-adding them

@isidorn As a result of discussions in #59984 and #62264 I have started removing and re-creating breakpoints in setBreakpointsRequest because we deemed it was the only way to attach IDs to them if I couldn’t supply the IDs immediately. However, I’m seeing a strange issue where breakpoints will become marked as unverified whenever I remove (a different) one, even though I create them with verified: true.

Here’s a fake setBreakpointsRequest implementation that shows the issue. All it does is sets them all as unverified from the original request, then events to remove each one, and then sends events to recreate them (this time, with IDs). As discussed, I send the response to the request before I send the removed/new events):

	private id = 1;
	protected async setBreakPointsRequest(
		response: DebugProtocol.SetBreakpointsResponse,
		args: DebugProtocol.SetBreakpointsArguments,
	): Promise<void> {
		const source: DebugProtocol.Source = args.source;
		let breakpoints: DebugProtocol.SourceBreakpoint[] = args.breakpoints;
		if (!breakpoints)
			breakpoints = [];

		// Send back all unverified breakpoints.
		response.body = {
			breakpoints: breakpoints.map((_) => ({
				verified: false,
			} as DebugProtocol.Breakpoint)),
		};
		this.sendResponse(response);

		// Now remove them all.
		breakpoints.forEach((breakpoint) => {
			this.sendEvent(new BreakpointEvent("removed", {
				column: breakpoint.column,
				line: breakpoint.line,
				source: new Source(args.source.name, args.source.path),
			} as DebugProtocol.Breakpoint));
		});

		// Now re-add them all.
		breakpoints.forEach((breakpoint) => {
			this.sendEvent(new BreakpointEvent("new", {
				column: breakpoint.column,
				id: this.id++,
				line: breakpoint.line,
				source: new Source(args.source.name, args.source.path),
				verified: true,
			} as DebugProtocol.Breakpoint));
		});
	}

And here’s a video showing that whenever I remove a breakpoint, the others all become unverified (yet whenever I add one, they always become verified). I can’t figure out why this happens.

breakpoints

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Comments: 33 (32 by maintainers)

Most upvoted comments

The issue is not the low-level send but the fact that our debugger implementation is asynchronous. After a request is sent to the DA and while we are waiting for the response, an event can be received.

So our breakpoint event handler does not check whether all outstanding setBreakpoint responses have been processed (and the data structures are updated) before it handles the breakpoint event.

There are two ways to fix this:

  • by processing all responses and events in a strict sequential order. So an event would only be processed if all previous events and request/responses have been processed.
  • by protecting critical data structures against concurrent modification. So we would block “breakpointEvents” processing while at least one “setBreakpointRequest” is still waiting for its response (because both modify the set of breakpoints). See https://github.com/compulim/promise-critical-section for how this can be done.

We need to fix this issue and I suggest to use the second approach as it only limits concurrency where needed.