Fixing race condition on breakpoints (#536)

asimgunes · web-flow · commit 8d22efbafa20 · 2026-05-22T14:32:25.000+01:00
diff --git a/package.json b/package.json
@@ -63,6 +63,7 @@
   "dependencies": {
     "@vscode/debugadapter": "^1.68.0",
     "@vscode/debugprotocol": "^1.68.0",
+    "async-mutex": "^0.5.0",
     "node-addon-api": "^8.4.0",
     "serialport": "^13.0.0"
   },
diff --git a/src/gdb/GDBDebugSessionBase.ts b/src/gdb/GDBDebugSessionBase.ts
@@ -27,6 +27,7 @@ import {
     Event,
 } from '@vscode/debugadapter';
 import { DebugProtocol } from '@vscode/debugprotocol';
+import { Mutex } from 'async-mutex';
 import { ContinuedEvent } from '../events/continuedEvent';
 import { StoppedEvent } from '../events/stoppedEvent';
 import { VarObjType } from '../varManager';
@@ -191,6 +192,12 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
     // first of them pauses, and the last to complete resumes
     protected pauseCount = 0;
 
+    // Serializes the bodies of the breakpoint request handlers so their
+    // read-modify-write of GDB's global breakpoint list cannot interleave.
+    // pauseIfNeeded/continueIfNeeded only coalesces the target pause, it does
+    // not prevent concurrent handlers from acting on stale snapshots.
+    protected breakpointMutex = new Mutex();
+
     // Pending requests to pause a thread silently (without sending stopped event).
     // At the moment only used with pauseIfRunning().
     protected silentPauseRequests: PendingPauseRequest[] = [];
@@ -917,6 +924,15 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
     protected async setDataBreakpointsRequest(
         response: DebugProtocol.SetDataBreakpointsResponse,
         args: DebugProtocol.SetDataBreakpointsArguments
+    ): Promise<void> {
+        await this.breakpointMutex.runExclusive(() =>
+            this.doSetDataBreakpointsRequest(response, args)
+        );
+    }
+
+    protected async doSetDataBreakpointsRequest(
+        response: DebugProtocol.SetDataBreakpointsResponse,
+        args: DebugProtocol.SetDataBreakpointsArguments
     ): Promise<void> {
         // If this is the first time sending this breakpoint and there are no breakpoints to set,
         // do not change state of the target by avoiding an unnecessary pause
@@ -1027,18 +1043,17 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
         response: DebugProtocol.SetInstructionBreakpointsResponse,
         args: DebugProtocol.SetInstructionBreakpointsArguments
     ): Promise<void> {
-        try {
-            return await this.doSetInstructionBreakpointsRequest(
-                response,
-                args
-            );
-        } catch (err) {
-            this.sendErrorResponse(
-                response,
-                1,
-                err instanceof Error ? err.message : String(err)
-            );
-        }
+        await this.breakpointMutex.runExclusive(async () => {
+            try {
+                await this.doSetInstructionBreakpointsRequest(response, args);
+            } catch (err) {
+                this.sendErrorResponse(
+                    response,
+                    1,
+                    err instanceof Error ? err.message : String(err)
+                );
+            }
+        });
     }
 
     protected async doSetInstructionBreakpointsRequest(
@@ -1166,6 +1181,15 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
     protected async setBreakPointsRequest(
         response: DebugProtocol.SetBreakpointsResponse,
         args: DebugProtocol.SetBreakpointsArguments
+    ): Promise<void> {
+        await this.breakpointMutex.runExclusive(() =>
+            this.doSetBreakPointsRequest(response, args)
+        );
+    }
+
+    protected async doSetBreakPointsRequest(
+        response: DebugProtocol.SetBreakpointsResponse,
+        args: DebugProtocol.SetBreakpointsArguments
     ): Promise<void> {
         // If this is the first time sending this breakpoint and there are no breakpoints to set,
         // do not change state of the target by avoiding an unnecessary pause
@@ -1417,6 +1441,15 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
     protected async setFunctionBreakPointsRequest(
         response: DebugProtocol.SetFunctionBreakpointsResponse,
         args: DebugProtocol.SetFunctionBreakpointsArguments
+    ) {
+        await this.breakpointMutex.runExclusive(() =>
+            this.doSetFunctionBreakPointsRequest(response, args)
+        );
+    }
+
+    protected async doSetFunctionBreakPointsRequest(
+        response: DebugProtocol.SetFunctionBreakpointsResponse,
+        args: DebugProtocol.SetFunctionBreakpointsArguments
     ) {
         // If this is the first time sending this breakpoint and there are no breakpoints to set,
         // do not change state of the target by avoiding an unnecessary pause
diff --git a/src/integration-tests/breakpointMutex.spec.ts b/src/integration-tests/breakpointMutex.spec.ts
@@ -0,0 +1,120 @@
+/*********************************************************************
+ * Copyright (c) 2026 Renesas Electronics Corporation and others
+ *
+ * This program and the accompanying materials are made
+ * available under the terms of the Eclipse Public License 2.0
+ * which is available at https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *********************************************************************/
+
+import { expect } from 'chai';
+import * as sinon from 'sinon';
+import { DebugProtocol } from '@vscode/debugprotocol';
+import { GDBDebugSessionBase } from '../gdb/GDBDebugSessionBase';
+import { IGDBBackendFactory } from '../types/gdb';
+
+/**
+ * Minimal subclass that only exists to make the protected breakpoint wrapper
+ * handlers callable from test code. The `do*` implementations are replaced
+ * per-test via sinon stubs so we can observe how the wrapper-level mutex
+ * serializes them without coupling the test to a fixed class hierarchy.
+ */
+class TestSession extends GDBDebugSessionBase {
+    constructor() {
+        super({} as IGDBBackendFactory);
+    }
+
+    public callSetBreakPointsRequest(
+        response: DebugProtocol.SetBreakpointsResponse,
+        args: DebugProtocol.SetBreakpointsArguments
+    ) {
+        return this.setBreakPointsRequest(response, args);
+    }
+
+    public callSetInstructionBreakpointsRequest(
+        response: DebugProtocol.SetInstructionBreakpointsResponse,
+        args: DebugProtocol.SetInstructionBreakpointsArguments
+    ) {
+        return this.setInstructionBreakpointsRequest(response, args);
+    }
+}
+
+const emptyResponse = <T>() => ({ body: {} as never }) as T;
+
+describe('breakpoint handler serialization', function () {
+    let sandbox: sinon.SinonSandbox;
+    let session: TestSession;
+    let inFlight: number;
+    let maxConcurrent: number;
+
+    beforeEach(function () {
+        sandbox = sinon.createSandbox();
+        session = new TestSession();
+        inFlight = 0;
+        maxConcurrent = 0;
+
+        const simulateWork = async () => {
+            inFlight++;
+            maxConcurrent = Math.max(maxConcurrent, inFlight);
+            await new Promise((resolve) => setTimeout(resolve, 20));
+            inFlight--;
+        };
+
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        const s = session as any;
+        sandbox.stub(s, 'doSetBreakPointsRequest').callsFake(simulateWork);
+        sandbox
+            .stub(s, 'doSetFunctionBreakPointsRequest')
+            .callsFake(simulateWork);
+        sandbox.stub(s, 'doSetDataBreakpointsRequest').callsFake(simulateWork);
+        sandbox
+            .stub(s, 'doSetInstructionBreakpointsRequest')
+            .callsFake(simulateWork);
+    });
+
+    afterEach(function () {
+        sandbox.restore();
+    });
+
+    it('serializes two concurrent setBreakPointsRequest calls', async function () {
+        await Promise.all([
+            session.callSetBreakPointsRequest(
+                emptyResponse<DebugProtocol.SetBreakpointsResponse>(),
+                {
+                    source: { path: 'a.c' },
+                    breakpoints: [{ line: 1 }],
+                }
+            ),
+            session.callSetBreakPointsRequest(
+                emptyResponse<DebugProtocol.SetBreakpointsResponse>(),
+                {
+                    source: { path: 'a.c' },
+                    breakpoints: [{ line: 2 }],
+                }
+            ),
+        ]);
+
+        expect(maxConcurrent).to.equal(1);
+    });
+
+    it('serializes setBreakPointsRequest with setInstructionBreakpointsRequest', async function () {
+        await Promise.all([
+            session.callSetBreakPointsRequest(
+                emptyResponse<DebugProtocol.SetBreakpointsResponse>(),
+                {
+                    source: { path: 'a.c' },
+                    breakpoints: [{ line: 1 }],
+                }
+            ),
+            session.callSetInstructionBreakpointsRequest(
+                emptyResponse<DebugProtocol.SetInstructionBreakpointsResponse>(),
+                {
+                    breakpoints: [{ instructionReference: '0x1000' }],
+                }
+            ),
+        ]);
+
+        expect(maxConcurrent).to.equal(1);
+    });
+});
diff --git a/yarn.lock b/yarn.lock
@@ -1013,6 +1013,13 @@ assertion-error@^2.0.1:
   resolved "https://registry.yarnpkg.com/assertion-error/-/assertion-error-2.0.1.tgz#f641a196b335690b1070bf00b6e7593fec190bf7"
   integrity sha512-Izi8RQcffqCeNVgFigKli1ssklIbpHnCYc6AknXGYoB6grJqyeby7jv12JUQgmTAnIDnbck1uxksT4dzN3PWBA==
 
+async-mutex@^0.5.0:
+  version "0.5.0"
+  resolved "https://registry.yarnpkg.com/async-mutex/-/async-mutex-0.5.0.tgz#353c69a0b9e75250971a64ac203b0ebfddd75482"
+  integrity sha512-1A94B18jkJ3DYq284ohPxoXbfTA5HsQ7/Mf4DEhcyLx3Bz27Rh59iScbB6EPiP+B+joue6YCxcMXSbFC1tZKwA==
+  dependencies:
+    tslib "^2.4.0"
+
 available-typed-arrays@^1.0.7:
   version "1.0.7"
   resolved "https://registry.yarnpkg.com/available-typed-arrays/-/available-typed-arrays-1.0.7.tgz#a5cc375d6a03c2efc87a553f3e0b1522def14846"
@@ -4541,6 +4548,11 @@ ts-node@^10.9.2:
     v8-compile-cache-lib "^3.0.1"
     yn "3.1.1"
 
+tslib@^2.4.0:
+  version "2.8.1"
+  resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.8.1.tgz#612efe4ed235d567e8aba5f2a5fab70280ade83f"
+  integrity sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==
+
 tty-browserify@0.0.1:
   version "0.0.1"
   resolved "https://registry.yarnpkg.com/tty-browserify/-/tty-browserify-0.0.1.tgz#3f05251ee17904dfd0677546670db9651682b811"
