Discard breakpoints that got deleted before verified (#138)

* Discard breakpoints that got deleted before verified

* Add log entry for when we removed the breakpoint

* disable coveralls publishing

.github/workflows/build.yml

@@ -25,7 +25,7 @@ jobs:
       - run: npm run build
       - run: npm run lint
       - run: npm run test
-      - run: npm run publish-coverage
+      #- run: npm run publish-coverage
   npm-release:
     #only run this task if a tag starting with 'v' was used to trigger this (i.e. a tagged release)
     if: startsWith(github.ref, 'refs/tags/v')

src/adapters/DebugProtocolAdapter.spec.ts

@@ -1,11 +1,11 @@
 
 import { expect } from 'chai';
-import { DebugProtocolClient } from '../debugProtocol/client/DebugProtocolClient';
-import { DebugProtocolAdapter, EvaluateContainer, KeyType } from './DebugProtocolAdapter';
+import type { DebugProtocolClient } from '../debugProtocol/client/DebugProtocolClient';
+import { DebugProtocolAdapter, KeyType } from './DebugProtocolAdapter';
 import { createSandbox } from 'sinon';
 import { VariableType, VariablesResponse } from '../debugProtocol/events/responses/VariablesResponse';
 import { DebugProtocolServer } from '../debugProtocol/server/DebugProtocolServer';
-import { util } from '../util';
+import { defer, util } from '../util';
 import { standardizePath as s } from 'brighterscript';
 import { DebugProtocolServerTestPlugin } from '../debugProtocol/DebugProtocolServerTestPlugin.spec';
 import { AllThreadsStoppedUpdate } from '../debugProtocol/events/updates/AllThreadsStoppedUpdate';
@@ -21,18 +21,26 @@ import { AddBreakpointsRequest } from '../debugProtocol/events/requests/AddBreak
 import { AddConditionalBreakpointsRequest } from '../debugProtocol/events/requests/AddConditionalBreakpointsRequest';
 import { AddConditionalBreakpointsResponse } from '../debugProtocol/events/responses/AddConditionalBreakpointsResponse';
 import { RemoveBreakpointsResponse } from '../debugProtocol/events/responses/RemoveBreakpointsResponse';
+import { BreakpointVerifiedUpdate } from '../debugProtocol/events/updates/BreakpointVerifiedUpdate';
+import { RemoveBreakpointsRequest } from '../debugProtocol/events/requests/RemoveBreakpointsRequest';
+import type { AfterSendRequestEvent } from '../debugProtocol/client/DebugProtocolClientPlugin';
 const sinon = createSandbox();
 
 let cwd = s`${process.cwd()}`;
 let tmpDir = s`${cwd}/.tmp`;
 let rootDir = s`${tmpDir}/rootDir`;
 const outDir = s`${tmpDir}/out`;
+/**
+ * A path to main.brs
+ */
+const srcPath = `${rootDir}/source/main.brs`;
 
 describe('DebugProtocolAdapter', () => {
     let adapter: DebugProtocolAdapter;
     let server: DebugProtocolServer;
     let client: DebugProtocolClient;
     let plugin: DebugProtocolServerTestPlugin;
+    let breakpointManager: BreakpointManager;
 
     beforeEach(async () => {
         sinon.stub(console, 'log').callsFake((...args) => { });
@@ -42,7 +50,7 @@ describe('DebugProtocolAdapter', () => {
         };
         const sourcemapManager = new SourceMapManager();
         const locationManager = new LocationManager(sourcemapManager);
-        const breakpointManager = new BreakpointManager(sourcemapManager, locationManager);
+        breakpointManager = new BreakpointManager(sourcemapManager, locationManager);
         const projectManager = new ProjectManager(breakpointManager, locationManager);
         projectManager.mainProject = new Project({
             rootDir: rootDir,
@@ -57,10 +65,6 @@ describe('DebugProtocolAdapter', () => {
         server = new DebugProtocolServer(options);
         plugin = server.plugins.add(new DebugProtocolServerTestPlugin());
         await server.start();
-
-        client = new DebugProtocolClient(options);
-        //disable logging for tests because they clutter the test output
-        client['logger'].logLevel = 'off';
     });
 
     afterEach(async () => {
@@ -76,6 +80,9 @@ describe('DebugProtocolAdapter', () => {
      */
     async function initialize() {
         await adapter.connect();
+        client = adapter['socketDebugger'];
+        //disable logging for tests because they clutter the test output
+        client['logger'].logLevel = 'off';
         await Promise.all([
             adapter.once('suspend'),
             plugin.server.sendUpdate(
@@ -128,6 +135,93 @@ describe('DebugProtocolAdapter', () => {
     });
 
     describe('syncBreakpoints', () => {
+        it('removes "added" breakpoints that show up after a breakpoint was already removed', async () => {
+            const bpId = 123;
+            const bpLine = 12;
+            await initialize();
+
+            //force the client to expect the device to verify breakpoints (instead of auto-verifying them as soon as seen)
+            client.protocolVersion = '3.2.0';
+
+            breakpointManager.setBreakpoint(srcPath, {
+                line: bpLine
+            });
+
+            let bpResponseDeferred = defer();
+
+            //once the breakpoint arrives at the server
+            let bpAtServerPromise = new Promise<void>((resolve) => {
+                let handled = false;
+                const tempPlugin = server.plugins.add({
+                    provideResponse: (event) => {
+                        if (!handled && event.request instanceof AddBreakpointsRequest) {
+                            handled = true;
+                            //resolve the outer promise
+                            resolve();
+                            //return a deferred promise for us to flush later
+                            return bpResponseDeferred.promise;
+                        }
+                    }
+                });
+            });
+
+            plugin.pushResponse(
+                AddBreakpointsResponse.fromJson({
+                    requestId: 1,
+                    breakpoints: [{
+                        id: bpId,
+                        errorCode: ErrorCode.OK,
+                        ignoreCount: 0
+                    }]
+                })
+            );
+            //sync the breakpoints to mark this one as "sent to device"
+            void adapter.syncBreakpoints();
+            //wait for the request to arrive at the server (it will be stuck pending until we resolve the bpResponseDeferred)
+            await bpAtServerPromise;
+
+            //delete the breakpoint (before we ever got the deviceId from the server)
+            breakpointManager.replaceBreakpoints(srcPath, []);
+
+            //sync the breakpoints again, forcing the bp to be fully deleted
+            let syncPromise = adapter.syncBreakpoints();
+            //since the breakpoints were deleted before getting deviceIDs, there should be no request sent
+            bpResponseDeferred.resolve();
+            //wait for the second sync to finish
+            await syncPromise;
+
+            //response for the "remove breakpoints" request triggered later
+            plugin.pushResponse(
+                RemoveBreakpointsResponse.fromJson({
+                    requestId: 1,
+                    breakpoints: [{
+                        id: bpId,
+                        errorCode: ErrorCode.OK,
+                        ignoreCount: 0
+                    }]
+                })
+            );
+
+            //listen for the next sent RemoveBreakpointsRequest
+            const sentRequestPromise = client.plugins.onceIf<AfterSendRequestEvent<RemoveBreakpointsRequest>>('afterSendRequest', (event) => {
+                return event.request instanceof RemoveBreakpointsRequest;
+            }, 0);
+
+            //now push the "bp verified" event
+            //the client should recognize that these breakpoints aren't avaiable client-side, and ask the server to delete them
+            await server.sendUpdate(
+                BreakpointVerifiedUpdate.fromJson({
+                    breakpoints: [{
+                        id: bpId
+                    }]
+                })
+            );
+
+            //wait for the request to be sent
+            expect(
+                (await sentRequestPromise).request?.data.breakpointIds
+            ).to.eql([bpId]);
+        });
 
         it('excludes non-numeric breakpoint IDs', async () => {
             await initialize();

src/adapters/DebugProtocolAdapter.ts

@@ -283,9 +283,19 @@ export class DebugProtocolAdapter {
 
             //handle when the device verifies breakpoints
             this.socketDebugger.on('breakpoints-verified', (event) => {
+                let unverifiableDeviceIds = [] as number[];
+
                 //mark the breakpoints as verified
                 for (let breakpoint of event?.breakpoints ?? []) {
-                    this.breakpointManager.verifyBreakpoint(breakpoint.id, true);
+                    const success = this.breakpointManager.verifyBreakpoint(breakpoint.id, true);
+                    if (!success) {
+                        unverifiableDeviceIds.push(breakpoint.id);
+                    }
+                }
+                //if there were any unsuccessful breakpoint verifications, we need to ask the device to delete those breakpoints as they've gone missing on our side
+                if (unverifiableDeviceIds.length > 0) {
+                    this.logger.warn('Could not find breakpoints to verify. Removing from device:', { deviceBreakpointIds: unverifiableDeviceIds });
+                    void this.socketDebugger.removeBreakpoints(unverifiableDeviceIds);
                 }
                 this.emit('breakpoints-verified', event);
             });

src/debugProtocol/PluginInterface.ts

@@ -28,6 +28,8 @@ export default class PluginInterface<TPlugin> {
 
     /**
      * Add a plugin to the end of the list of plugins
+     * @param plugin the plugin
+     * @param priority the priority for the plugin. Lower number means higher priority. (ex: 1 executes before 5)
      */
     public add<T extends TPlugin = TPlugin>(plugin: T, priority = 1) {
         const container = {
@@ -44,6 +46,38 @@ export default class PluginInterface<TPlugin> {
         return plugin;
     }
 
+    /**
+     * Adds a temporary plugin with a single event hook, and resolve a promise with the event from the next occurance of that event.
+     * Once the event fires for the first time, the plugin is unregistered.
+     * @param eventName the name of the event to subscribe to
+     * @param priority the priority for this event. Lower number means higher priority. (ex: 1 executes before 5)
+     */
+    public once<TEventType>(eventName: keyof TPlugin, priority = 1): Promise<TEventType> {
+        return this.onceIf(eventName, () => true, priority);
+    }
+
+    /**
+     * Adds a temporary plugin with a single event hook, and resolve a promise with the event from the next occurance of that event.
+     * Once the event fires for the first time and the matcher evaluates to true, the plugin is unregistered.
+     * @param eventName the name of the event to subscribe to
+     * @param matcher a function to call that, when true, will deregister this hander and return the event
+     * @param priority the priority for this event. Lower number means higher priority. (ex: 1 executes before 5)
+     */
+    public onceIf<TEventType>(eventName: keyof TPlugin, matcher: (TEventType) => boolean, priority = 1): Promise<TEventType> {
+        return new Promise((resolve) => {
+            const tempPlugin = {} as any;
+            tempPlugin[eventName] = (event) => {
+                if (matcher(event)) {
+                    //remove the temp plugin
+                    this.remove(tempPlugin);
+                    //resolve the promise with this event
+                    resolve(event);
+                }
+            };
+            this.add(tempPlugin, priority);
+        }) as any;
+    }
+
     /**
      * Remove the specified plugin
      */

src/debugProtocol/client/DebugProtocolClientPlugin.ts

@@ -31,11 +31,11 @@ export interface ProvideResponseOrUpdateEvent {
     responseOrUpdate?: ProtocolResponse | ProtocolUpdate;
 }
 
-export interface BeforeSendRequestEvent {
+export interface BeforeSendRequestEvent<TRequest extends ProtocolRequest = ProtocolRequest> {
     client: DebugProtocolClient;
-    request: ProtocolRequest;
+    request: TRequest;
 }
-export type AfterSendRequestEvent = BeforeSendRequestEvent;
+export type AfterSendRequestEvent<TRequest extends ProtocolRequest = ProtocolRequest> = BeforeSendRequestEvent<TRequest>;
 
 export interface OnUpdateEvent {
     client: DebugProtocolClient;

src/debugProtocol/server/DebugProtocolServerPlugin.ts

@@ -1,13 +1,14 @@
 import type { DebugProtocolServer } from './DebugProtocolServer';
 import type { Socket } from 'net';
 import type { ProtocolRequest, ProtocolResponse } from '../events/ProtocolEvent';
+import { DebugProtocolServerTestPlugin } from '../DebugProtocolServerTestPlugin.spec';
 
 export interface ProtocolServerPlugin {
     onServerStart?: Handler<OnServerStartEvent>;
     onClientConnected?: Handler<OnClientConnectedEvent>;
 
     provideRequest?: Handler<ProvideRequestEvent>;
-    provideResponse: Handler<ProvideResponseEvent>;
+    provideResponse?: Handler<ProvideResponseEvent>;
 
     beforeSendResponse?: Handler<BeforeSendResponseEvent>;
     afterSendResponse?: Handler<AfterSendResponseEvent>;
@@ -46,4 +47,3 @@ export interface BeforeSendResponseEvent {
 export type AfterSendResponseEvent = BeforeSendResponseEvent;
 
 export type Handler<T, R = void> = (event: T) => R;
-

src/managers/BreakpointManager.ts

@@ -188,8 +188,11 @@ export class BreakpointManager {
         if (breakpoint) {
             breakpoint.verified = isVerified;
             this.queueVerifyEvent(breakpoint.hash);
+            return true;
+        } else {
+            //couldn't find the breakpoint. return false so the caller can handle that properly
+            return false;
         }
-        //TODO handle the else case, (might be caused by timing issues?)
     }
 
     /**