GLSP-1438: Improve GLSPClient default implementations (#402)

- Introduce `onCurrentStateChanged` event for `GLSPClient`
- Ensure that `start` and `initializeServer` behave as expected if they are called 
  again while the previous promise is still pending.
- Restructure members of the default implementations so that get/setters and their _property are grouped together
- Introduce a dedicated namespace for the `Event` interface with utility functions to
  - listen on a certain event exactly once
   - waitUntil a certain event is fired the next time
- Adapt test cases to verify new behavior

Also:
- Update `GLSPActionDispatcher` to use direct injection for registry instead of async provider

packages/client/src/base/action-dispatcher.ts

@@ -16,12 +16,14 @@
 import {
     Action,
     ActionDispatcher,
+    ActionHandlerRegistry,
     EMPTY_ROOT,
     GModelRoot,
     IActionDispatcher,
     RequestAction,
     ResponseAction,
-    SetModelAction
+    SetModelAction,
+    TYPES
 } from '@eclipse-glsp/sprotty';
 import { inject, injectable } from 'inversify';
 import { GLSPActionHandlerRegistry } from './action-handler-registry';
@@ -37,6 +39,12 @@ export class GLSPActionDispatcher extends ActionDispatcher implements IGModelRoo
     @inject(ModelInitializationConstraint)
     protected initializationConstraint: ModelInitializationConstraint;
 
+    @inject(ActionHandlerRegistry)
+    protected override actionHandlerRegistry: ActionHandlerRegistry;
+
+    /** @deprecated No longer in used. The {@link ActionHandlerRegistry} is now directly injected */
+    // eslint-disable-next-line deprecation/deprecation
+    @inject(TYPES.ActionHandlerRegistryProvider) protected override actionHandlerRegistryProvider: () => Promise<ActionHandlerRegistry>;
     protected postUpdateQueue: Action[] = [];
 
     override initialize(): Promise<void> {
@@ -47,10 +55,8 @@ export class GLSPActionDispatcher extends ActionDispatcher implements IGModelRoo
     }
 
     protected async doInitialize(): Promise<void> {
-        const registry = await this.actionHandlerRegistryProvider();
-        this.actionHandlerRegistry = registry;
-        if (registry instanceof GLSPActionHandlerRegistry) {
-            registry.initialize();
+        if (this.actionHandlerRegistry instanceof GLSPActionHandlerRegistry) {
+            this.actionHandlerRegistry.initialize();
         }
         this.handleAction(SetModelAction.create(EMPTY_ROOT)).catch(() => {
             /* Logged in handleAction method */

packages/protocol/src/client-server-protocol/base-glsp-client.spec.ts

@@ -70,31 +70,48 @@ describe('Node GLSP Client', () => {
     describe('start', () => {
         it('should fail if no server is configured', async () => {
             resetClient(false);
+            const stateChangeHandler = sinon.spy();
+            client.onCurrentStateChanged(stateChangeHandler);
             client.setStartupTimeout(5);
             await expectToThrowAsync(() => client.start());
             expect(client.currentState).to.be.equal(ClientState.StartFailed);
+            expect(stateChangeHandler.calledWith(ClientState.StartFailed)).to.be.true;
         });
         it('Should resolve when server is configured', async () => {
             resetClient(false);
+            const stateChangeHandler = sinon.spy();
+            client.onCurrentStateChanged(stateChangeHandler);
+            const started = client.start();
+            expect(client.currentState).to.be.equal(ClientState.Starting);
+            expect(stateChangeHandler.calledWith(ClientState.Starting)).to.be.true;
             client.configureServer(server);
-            const result = await client.start();
-            expect(result).to.be.undefined;
+            await started;
             expect(client.currentState).to.be.equal(ClientState.Running);
+            expect(stateChangeHandler.calledWith(ClientState.Running)).to.be.true;
         });
     });
 
     describe('stop & onStop', () => {
-        beforeEach(() => resetClient());
         it('onStop should not resolve if stop has not been called', () => {
+            resetClient();
             expect(util.inspect(client.onStop())).to.include('pending');
         });
         it('should be in stopped state and onStop should resolve', async () => {
+            resetClient();
             expect(client.currentState).to.be.not.equal(ClientState.Stopped);
-            const stopResult = await client.stop();
-            expect(stopResult).to.be.undefined;
+            const stateChangeHandler = sinon.spy();
+            client.onCurrentStateChanged(stateChangeHandler);
+            await client.stop();
+            expect(client.currentState).to.be.equal(ClientState.Stopped);
+            expect(stateChangeHandler.calledWith(ClientState.Stopping)).to.be.true;
+            expect(stateChangeHandler.calledWith(ClientState.Stopped)).to.be.true;
+            expect(server.shutdown.calledOnce).to.be.true;
+        });
+        it('should only stop a running client once, if stop is called multiple times ', async () => {
+            resetClient();
+            client.stop();
+            await client.stop();
             expect(client.currentState).to.be.equal(ClientState.Stopped);
-            const onStopResult = await client.onStop();
-            expect(onStopResult).to.be.undefined;
             expect(server.shutdown.calledOnce).to.be.true;
         });
     });
@@ -104,12 +121,14 @@ describe('Node GLSP Client', () => {
             resetClient(false);
             await expectToThrowAsync(() => client.initializeServer({ applicationId: '', protocolVersion: '' }));
             expect(server.initialize.called).to.be.false;
+            expect(client.initializeResult).to.be.undefined;
         });
         it('should fail if client is not running', async () => {
             resetClient(false);
             client.configureServer(server);
             await expectToThrowAsync(() => client.initializeServer({ applicationId: '', protocolVersion: '' }));
             expect(server.initialize.called).to.be.false;
+            expect(client.initializeResult).to.be.undefined;
         });
         it('should invoke the corresponding server method', async () => {
             resetClient();
@@ -122,17 +141,17 @@ describe('Node GLSP Client', () => {
             expect(client.initializeResult).to.be.equal(result);
         });
         it('should return cached result on consecutive invocation', async () => {
-            await resetClient();
+            resetClient();
             const expectedResult = { protocolVersion: '1.0.0', serverActions: {} };
             const params = { applicationId: 'id', protocolVersion: '1.0.0' };
             server.initialize.returns(Promise.resolve(expectedResult));
-            client['_initializeResult'] = expectedResult;
+            client.initializeServer(params);
             const result = await client.initializeServer(params);
             expect(result).to.be.deep.equal(client.initializeResult);
-            expect(server.initialize.called).to.be.false;
+            expect(server.initialize.calledOnce).to.be.true;
         });
         it('should fire event on first invocation', async () => {
-            await resetClient();
+            resetClient();
             const expectedResult = { protocolVersion: '1.0.0', serverActions: {} };
             const params = { applicationId: 'id', protocolVersion: '1.0.0' };
             server.initialize.returns(Promise.resolve(expectedResult));
@@ -144,6 +163,19 @@ describe('Node GLSP Client', () => {
             await client.initializeServer(params);
             expect(eventHandlerSpy.calledOnceWith(expectedResult)).to.be.true;
         });
+        it('should not use cached result on consecutive invocation if previous invocation errored', async () => {
+            resetClient();
+            const expectedResult = { protocolVersion: '1.0.0', serverActions: {} };
+            const params = { applicationId: 'id', protocolVersion: '1.0.0' };
+            server.initialize.throws(new Error('error'));
+            expectToThrowAsync(() => client.initializeServer(params));
+            expect(client.initializeResult).to.be.undefined;
+            server.initialize.returns(Promise.resolve(expectedResult));
+            const result = await client.initializeServer(params);
+            expect(result).to.be.deep.equal(expectedResult);
+            expect(server.initialize.calledTwice).to.be.true;
+            expect(client.initializeResult).to.be.equal(result);
+        });
     });
 
     describe('initializeClientSession', () => {

packages/protocol/src/client-server-protocol/base-glsp-client.ts

@@ -30,21 +30,49 @@ export const GLOBAL_HANDLER_ID = '*';
  * directly communicates with a given {@link GLSPServer} instance.
  */
 export class BaseGLSPClient implements GLSPClient {
-    protected state: ClientState;
-    protected _server?: GLSPServer;
     protected serverDeferred = new Deferred<GLSPServer>();
     protected onStartDeferred = new Deferred<void>();
     protected onStopDeferred = new Deferred<void>();
     readonly proxy: GLSPClientProxy;
     protected startupTimeout = 1500;
     protected actionMessageHandlers: Map<string, ActionMessageHandler[]> = new Map([[GLOBAL_HANDLER_ID, []]]);
-    protected _initializeResult: InitializeResult;
+    protected pendingServerInitialize?: Promise<InitializeResult>;
 
     protected onServerInitializedEmitter = new Emitter<InitializeResult>();
     get onServerInitialized(): Event<InitializeResult> {
         return this.onServerInitializedEmitter.event;
     }
 
+    protected onCurrentStateChangedEmitter = new Emitter<ClientState>();
+    get onCurrentStateChanged(): Event<ClientState> {
+        return this.onCurrentStateChangedEmitter.event;
+    }
+
+    protected _state: ClientState;
+    protected set state(state: ClientState) {
+        if (this._state !== state) {
+            this._state = state;
+            this.onCurrentStateChangedEmitter.fire(state);
+        }
+    }
+    protected get state(): ClientState {
+        return this._state;
+    }
+
+    protected _server?: GLSPServer;
+    protected get checkedServer(): GLSPServer {
+        this.checkState();
+        if (!this._server) {
+            throw new Error(`No server is configured for GLSPClient with id '${this.id}'`);
+        }
+        return this._server;
+    }
+
+    protected _initializeResult?: InitializeResult;
+    get initializeResult(): InitializeResult | undefined {
+        return this._initializeResult;
+    }
+
     constructor(protected options: GLSPClient.Options) {
         this.state = ClientState.Initial;
         this.proxy = this.createProxy();
@@ -71,7 +99,7 @@ export class BaseGLSPClient implements GLSPClient {
     }
 
     start(): Promise<void> {
-        if (this.state === ClientState.Running) {
+        if (this.state === ClientState.Running || this.state === ClientState.Starting) {
             return this.onStartDeferred.promise;
         }
 
@@ -96,15 +124,25 @@ export class BaseGLSPClient implements GLSPClient {
     }
 
     async initializeServer(params: InitializeParameters): Promise<InitializeResult> {
-        if (!this._initializeResult) {
+        if (this.initializeResult) {
+            return this.initializeResult;
+        } else if (this.pendingServerInitialize) {
+            return this.pendingServerInitialize;
+        }
+
+        const initializeDeferred = new Deferred<InitializeResult>();
+        try {
+            this.pendingServerInitialize = initializeDeferred.promise;
             this._initializeResult = await this.checkedServer.initialize(params);
             this.onServerInitializedEmitter.fire(this._initializeResult);
+            initializeDeferred.resolve(this._initializeResult);
+            this.pendingServerInitialize = undefined;
+        } catch (error) {
+            initializeDeferred.reject(error);
+            this._initializeResult = undefined;
+            this.pendingServerInitialize = undefined;
         }
-        return this._initializeResult;
-    }
-
-    get initializeResult(): InitializeResult | undefined {
-        return this._initializeResult;
+        return initializeDeferred.promise;
     }
 
     initializeClientSession(params: InitializeClientSessionParameters): Promise<void> {
@@ -174,14 +212,6 @@ export class BaseGLSPClient implements GLSPClient {
         }
     }
 
-    protected get checkedServer(): GLSPServer {
-        this.checkState();
-        if (!this._server) {
-            throw new Error(`No server is configured for GLSPClient with id '${this.id}'`);
-        }
-        return this._server;
-    }
-
     setStartupTimeout(ms: number): void {
         this.startupTimeout = ms;
     }

packages/protocol/src/client-server-protocol/glsp-client.ts

@@ -74,6 +74,10 @@ export interface GLSPClient {
      * Current client state.
      */
     readonly currentState: ClientState;
+    /**
+     * Event that is fired whenever the client state changes.
+     */
+    readonly onCurrentStateChanged: Event<ClientState>;
 
     /**
      * Initializes the client and the server connection. During the start procedure the client is in the
@@ -134,6 +138,7 @@ export interface GLSPClient {
     /**
      * Stops the client and disposes unknown resources. During the stop procedure the client is in the `Stopping` state and will
      * transition to either `Stopped` or `ServerError`.
+     * Calling the method if client is already stopped has no effect.
      *
      * @returns A promise that resolves after the server was stopped and disposed.
      */