chore(web): refactor xtext client

8 files changed, 130 insertions, 75 deletions

diff --git a/language-web/src/main/js/editor/EditorStore.ts b/language-web/src/main/js/editor/EditorStore.ts
index 8788e00f..ba31efcb 100644
--- a/language-web/src/main/js/editor/EditorStore.ts
+++ b/language-web/src/main/js/editor/EditorStore.ts
@@ -56,11 +56,11 @@ import { XtextClient } from '../xtext/XtextClient';
 const log = getLogger('editor.EditorStore');
 
 export class EditorStore {
-  themeStore;
+  private readonly themeStore;
 
   state: EditorState;
 
-  client: XtextClient;
+  private readonly client: XtextClient;
 
   showLineNumbers = false;
 
@@ -74,11 +74,11 @@ export class EditorStore {
 
   infoCount = 0;
 
-  readonly defaultDispatcher = (tr: Transaction): void => {
+  private readonly defaultDispatcher = (tr: Transaction): void => {
     this.onTransaction(tr);
   };
 
-  dispatcher = this.defaultDispatcher;
+  private dispatcher = this.defaultDispatcher;
 
   constructor(initialValue: string, themeStore: ThemeStore) {
     this.themeStore = themeStore;
@@ -148,10 +148,7 @@ export class EditorStore {
       },
     );
     makeAutoObservable(this, {
-      themeStore: false,
       state: observable.ref,
-      defaultDispatcher: false,
-      dispatcher: false,
     });
   }
 
diff --git a/language-web/src/main/js/xtext/ContentAssistService.ts b/language-web/src/main/js/xtext/ContentAssistService.ts
index 8461ec7f..f085c5b1 100644
--- a/language-web/src/main/js/xtext/ContentAssistService.ts
+++ b/language-web/src/main/js/xtext/ContentAssistService.ts
@@ -61,9 +61,9 @@ function computeSpan(prefix: string, entryCount: number) {
 }
 
 export class ContentAssistService {
-  updateService: UpdateService;
+  private readonly updateService: UpdateService;
 
-  lastCompletion: CompletionResult | null = null;
+  private lastCompletion: CompletionResult | null = null;
 
   constructor(updateService: UpdateService) {
     this.updateService = updateService;
diff --git a/language-web/src/main/js/xtext/HighlightingService.ts b/language-web/src/main/js/xtext/HighlightingService.ts
index 451a3a52..fc3e9e53 100644
--- a/language-web/src/main/js/xtext/HighlightingService.ts
+++ b/language-web/src/main/js/xtext/HighlightingService.ts
@@ -7,9 +7,9 @@ import { isHighlightingResult } from './xtextServiceResults';
 const log = getLogger('xtext.ValidationService');
 
 export class HighlightingService {
-  private store: EditorStore;
+  private readonly store: EditorStore;
 
-  private updateService: UpdateService;
+  private readonly updateService: UpdateService;
 
   constructor(store: EditorStore, updateService: UpdateService) {
     this.store = store;
diff --git a/language-web/src/main/js/xtext/OccurrencesService.ts b/language-web/src/main/js/xtext/OccurrencesService.ts
index 804f5ba2..d1dec9e9 100644
--- a/language-web/src/main/js/xtext/OccurrencesService.ts
+++ b/language-web/src/main/js/xtext/OccurrencesService.ts
@@ -34,19 +34,19 @@ function transformOccurrences(regions: ITextRegion[]): IOccurrence[] {
 }
 
 export class OccurrencesService {
-  private store: EditorStore;
+  private readonly store: EditorStore;
 
-  private webSocketClient: XtextWebSocketClient;
+  private readonly webSocketClient: XtextWebSocketClient;
 
-  private updateService: UpdateService;
+  private readonly updateService: UpdateService;
 
   private hasOccurrences = false;
 
-  private findOccurrencesTimer = new Timer(() => {
+  private readonly findOccurrencesTimer = new Timer(() => {
     this.handleFindOccurrences();
   }, FIND_OCCURRENCES_TIMEOUT_MS);
 
-  private clearOccurrencesTimer = new Timer(() => {
+  private readonly clearOccurrencesTimer = new Timer(() => {
     this.clearOccurrences();
   }, CLEAR_OCCURRENCES_TIMEOUT_MS);
 
diff --git a/language-web/src/main/js/xtext/UpdateService.ts b/language-web/src/main/js/xtext/UpdateService.ts
index 838f9d5b..9b672e79 100644
--- a/language-web/src/main/js/xtext/UpdateService.ts
+++ b/language-web/src/main/js/xtext/UpdateService.ts
@@ -32,20 +32,27 @@ export class UpdateService {
 
   xtextStateId: string | null = null;
 
-  private store: EditorStore;
+  private readonly store: EditorStore;
 
+  /**
+   * The changes being synchronized to the server if a full or delta text update is running,
+   * `null` otherwise.
+   */
   private pendingUpdate: ChangeDesc | null = null;
 
+  /**
+   * Local changes not yet sychronized to the server and not part of the running update, if any.
+   */
   private dirtyChanges: ChangeDesc;
 
-  private webSocketClient: XtextWebSocketClient;
+  private readonly webSocketClient: XtextWebSocketClient;
 
-  private updatedCondition = new ConditionVariable(
+  private readonly updatedCondition = new ConditionVariable(
     () => this.pendingUpdate === null && this.xtextStateId !== null,
     WAIT_FOR_UPDATE_TIMEOUT_MS,
   );
 
-  private idleUpdateTimer = new Timer(() => {
+  private readonly idleUpdateTimer = new Timer(() => {
     this.handleIdleUpdate();
   }, UPDATE_TIMEOUT_MS);
 
@@ -56,9 +63,11 @@ export class UpdateService {
     this.webSocketClient = webSocketClient;
   }
 
-  onConnect(): Promise<void> {
+  onReconnect(): void {
     this.xtextStateId = null;
-    return this.updateFullText();
+    this.updateFullText().catch((error) => {
+      log.error('Unexpected error during initial update', error);
+    });
   }
 
   onTransaction(transaction: Transaction): void {
@@ -68,6 +77,14 @@ export class UpdateService {
     }
   }
 
+  /**
+   * Computes the summary of any changes happened since the last complete update.
+   *
+   * The result reflects any changes that happened since the `xtextStateId`
+   * version was uploaded to the server.
+   *
+   * @return the summary of changes since the last update
+   */
   computeChangesSinceLastUpdate(): ChangeDesc {
     return this.pendingUpdate?.composeDesc(this.dirtyChanges) || this.dirtyChanges;
   }
@@ -106,6 +123,15 @@ export class UpdateService {
     throw new Error('Full text update failed');
   }
 
+  /**
+   * Makes sure that the document state on the server reflects recent
+   * local changes.
+   *
+   * Performs either an update with delta text or a full text update if needed.
+   * If there are not local dirty changes, the promise resolves immediately.
+   *
+   * @return a promise resolving when the update is completed
+   */
   async update(): Promise<void> {
     await this.prepareForDeltaUpdate();
     const delta = this.computeDelta();
@@ -151,31 +177,36 @@ export class UpdateService {
       return [];
     }
     const delta = this.computeDelta();
-    if (delta === null) {
-      // Poscondition of `prepareForDeltaUpdate`: `xtextStateId !== null`
-      return this.doFetchContentAssist(params, this.xtextStateId as string);
-    }
-    log.trace('Editor delta', delta);
-    return this.withUpdate(async () => {
-      const result = await this.webSocketClient.send({
-        ...params,
-        requiredStateId: this.xtextStateId,
-        ...delta,
+    if (delta !== null) {
+      log.trace('Editor delta', delta);
+      const entries = await this.withUpdate(async () => {
+        const result = await this.webSocketClient.send({
+          ...params,
+          requiredStateId: this.xtextStateId,
+          ...delta,
+        });
+        if (isContentAssistResult(result)) {
+          return [result.stateId, result.entries];
+        }
+        if (isInvalidStateIdConflictResult(result)) {
+          const [newStateId] = await this.doFallbackToUpdateFullText();
+          // We must finish this state update transaction to prepare for any push events
+          // before querying for content assist, so we just return `null` and will query
+          // the content assist service later.
+          return [newStateId, null];
+        }
+        log.error('Unextpected content assist result with delta update', result);
+        throw new Error('Unexpexted content assist result with delta update');
       });
-      if (isContentAssistResult(result)) {
-        return [result.stateId, result.entries];
+      if (entries !== null) {
+        return entries;
       }
-      if (isInvalidStateIdConflictResult(result)) {
-        const [newStateId] = await this.doFallbackToUpdateFullText();
-        if (signal.aborted) {
-          return [newStateId, []];
-        }
-        const entries = await this.doFetchContentAssist(params, newStateId);
-        return [newStateId, entries];
+      if (signal.aborted) {
+        return [];
       }
-      log.error('Unextpected content assist result with delta update', result);
-      throw new Error('Unexpexted content assist result with delta update');
-    });
+    }
+    // Poscondition of `prepareForDeltaUpdate`: `xtextStateId !== null`
+    return this.doFetchContentAssist(params, this.xtextStateId as string);
   }
 
   private async doFetchContentAssist(params: Record<string, unknown>, expectedStateId: string) {
@@ -211,6 +242,27 @@ export class UpdateService {
     };
   }
 
+  /**
+   * Executes an asynchronous callback that updates the state on the server.
+   *
+   * Ensures that updates happen sequentially and manages `pendingUpdate`
+   * and `dirtyChanges` to reflect changes being synchronized to the server
+   * and not yet synchronized to the server, respectively.
+   *
+   * Optionally, `callback` may return a second value that is retured by this function.
+   *
+   * Once the remote procedure call to update the server state finishes
+   * and returns the new `stateId`, `callback` must return _immediately_
+   * to ensure that the local `stateId` is updated likewise to be able to handle
+   * push messages referring to the new `stateId` from the server.
+   * If additional work is needed to compute the second value in some cases,
+   * use `T | null` instead of `T` as a return type and signal the need for additional
+   * computations by returning `null`. Thus additional computations can be performed
+   * outside of the critical section.
+   *
+   * @param callback the asynchronous callback that updates the server state
+   * @return a promise resolving to the second value returned by `callback`
+   */
   private async withUpdate<T>(callback: () => Promise<[string, T]>): Promise<T> {
     if (this.pendingUpdate !== null) {
       throw new Error('Another update is pending, will not perform update');
@@ -239,6 +291,14 @@ export class UpdateService {
     }
   }
 
+  /**
+   * Ensures that there is some state available on the server (`xtextStateId`)
+   * and that there is not pending update.
+   *
+   * After this function resolves, a delta text update is possible.
+   *
+   * @return a promise resolving when there is a valid state id but no pending update
+   */
   private async prepareForDeltaUpdate() {
     // If no update is pending, but the full text hasn't been uploaded to the server yet,
     // we must start a full text upload.
diff --git a/language-web/src/main/js/xtext/ValidationService.ts b/language-web/src/main/js/xtext/ValidationService.ts
index 31c8f716..8e4934ac 100644
--- a/language-web/src/main/js/xtext/ValidationService.ts
+++ b/language-web/src/main/js/xtext/ValidationService.ts
@@ -8,9 +8,9 @@ import { isValidationResult } from './xtextServiceResults';
 const log = getLogger('xtext.ValidationService');
 
 export class ValidationService {
-  private store: EditorStore;
+  private readonly store: EditorStore;
 
-  private updateService: UpdateService;
+  private readonly updateService: UpdateService;
 
   constructor(store: EditorStore, updateService: UpdateService) {
     this.store = store;
diff --git a/language-web/src/main/js/xtext/XtextClient.ts b/language-web/src/main/js/xtext/XtextClient.ts
index 03b81b1c..28f3d0cc 100644
--- a/language-web/src/main/js/xtext/XtextClient.ts
+++ b/language-web/src/main/js/xtext/XtextClient.ts
@@ -16,21 +16,21 @@ import { XtextWebSocketClient } from './XtextWebSocketClient';
 const log = getLogger('xtext.XtextClient');
 
 export class XtextClient {
-  private webSocketClient: XtextWebSocketClient;
+  private readonly webSocketClient: XtextWebSocketClient;
 
-  private updateService: UpdateService;
+  private readonly updateService: UpdateService;
 
-  private contentAssistService: ContentAssistService;
+  private readonly contentAssistService: ContentAssistService;
 
-  private highlightingService: HighlightingService;
+  private readonly highlightingService: HighlightingService;
 
-  private validationService: ValidationService;
+  private readonly validationService: ValidationService;
 
-  private occurrencesService: OccurrencesService;
+  private readonly occurrencesService: OccurrencesService;
 
   constructor(store: EditorStore) {
     this.webSocketClient = new XtextWebSocketClient(
-      () => this.updateService.onConnect(),
+      () => this.updateService.onReconnect(),
       (resource, stateId, service, push) => this.onPush(resource, stateId, service, push),
     );
     this.updateService = new UpdateService(store, this.webSocketClient);
@@ -52,15 +52,17 @@ export class XtextClient {
     this.occurrencesService.onTransaction(transaction);
   }
 
-  private async onPush(resource: string, stateId: string, service: string, push: unknown) {
+  private onPush(resource: string, stateId: string, service: string, push: unknown) {
     const { resourceName, xtextStateId } = this.updateService;
     if (resource !== resourceName) {
       log.error('Unknown resource name: expected:', resourceName, 'got:', resource);
       return;
     }
     if (stateId !== xtextStateId) {
-      log.error('Unexpected xtext state id: expected:', xtextStateId, 'got:', resource);
-      await this.updateService.updateFullText();
+      log.error('Unexpected xtext state id: expected:', xtextStateId, 'got:', stateId);
+      // The current push message might be stale (referring to a previous state),
+      // so this is not neccessarily an error and there is no need to force-reconnect.
+      return;
     }
     switch (service) {
       case 'highlight':
diff --git a/language-web/src/main/js/xtext/XtextWebSocketClient.ts b/language-web/src/main/js/xtext/XtextWebSocketClient.ts
index 839d6518..488e4b3b 100644
--- a/language-web/src/main/js/xtext/XtextWebSocketClient.ts
+++ b/language-web/src/main/js/xtext/XtextWebSocketClient.ts
@@ -27,14 +27,14 @@ const REQUEST_TIMEOUT_MS = 1000;
 
 const log = getLogger('xtext.XtextWebSocketClient');
 
-type ReconnectHandler = () => Promise<void>;
+export type ReconnectHandler = () => void;
 
-type PushHandler = (
+export type PushHandler = (
   resourceId: string,
   stateId: string,
   service: string,
   data: unknown,
-) => Promise<void>;
+) => void;
 
 enum State {
   Initial,
@@ -47,29 +47,29 @@ enum State {
 }
 
 export class XtextWebSocketClient {
-  nextMessageId = 0;
+  private nextMessageId = 0;
 
-  connection!: WebSocket;
+  private connection!: WebSocket;
 
-  pendingRequests = new Map<string, PendingTask<unknown>>();
+  private readonly pendingRequests = new Map<string, PendingTask<unknown>>();
 
-  onReconnect: ReconnectHandler;
+  private readonly onReconnect: ReconnectHandler;
 
-  onPush: PushHandler;
+  private readonly onPush: PushHandler;
 
-  state = State.Initial;
+  private state = State.Initial;
 
-  reconnectTryCount = 0;
+  private reconnectTryCount = 0;
 
-  idleTimer = new Timer(() => {
+  private readonly idleTimer = new Timer(() => {
     this.handleIdleTimeout();
   }, BACKGROUND_IDLE_TIMEOUT_MS);
 
-  pingTimer = new Timer(() => {
+  private readonly pingTimer = new Timer(() => {
     this.sendPing();
   }, PING_TIMEOUT_MS);
 
-  reconnectTimer = new Timer(() => {
+  private readonly reconnectTimer = new Timer(() => {
     this.handleReconnect();
   });
 
@@ -115,9 +115,7 @@ export class XtextWebSocketClient {
       this.nextMessageId = 0;
       this.reconnectTryCount = 0;
       this.pingTimer.schedule();
-      this.onReconnect().catch((error) => {
-        log.error('Unexpected error in onReconnect handler', error);
-      });
+      this.onReconnect();
     });
     this.connection.addEventListener('error', (event) => {
       log.error('Unexpected websocket error', event);
@@ -264,9 +262,7 @@ export class XtextWebSocketClient {
         message.stateId,
         message.service,
         message.push,
-      ).catch((error) => {
-        log.error('Unexpected error in onPush handler', error);
-      });
+      );
     } else {
       log.error('Unexpected websocket message', message);
       this.forceReconnectOnError();