Fix race in VirtualDocument.dispose() / async update_documents()

Author: krassowski

packages/jupyterlab-lsp/src/adapters/adapter.ts

@@ -144,6 +144,7 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
     if (this.isDisposed) {
       return;
     }
+
     if (this.virtual_editor?.virtual_document) {
       this.disconnect_adapter(this.virtual_editor?.virtual_document);
     }
@@ -173,7 +174,6 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
     this.connection_manager = null;
     this.widget = null;
 
-    // actually disposed
     this.isDisposed = true;
   }
 
@@ -251,6 +251,10 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
    * public for use in tests (but otherwise could be private)
    */
   public update_documents() {
+    if (this.isDisposed) {
+      console.warn('Cannot update documents: adapter disposed');
+      return;
+    }
     return this.virtual_editor.virtual_document.update_manager.update_documents(
       this.editors.map(ce_editor => {
         return {
@@ -388,6 +392,11 @@ export abstract class WidgetAdapter<T extends IDocumentWidget> {
     document: VirtualDocument,
     is_init = false
   ) {
+    if (this.isDisposed) {
+      console.warn('Cannot swap document: adapter disposed');
+      return;
+    }
+
     // TODO only send the difference, using connection.sendSelectiveChange()
     let connection = this.connection_manager.connections.get(
       virtual_document.id_path

packages/jupyterlab-lsp/src/adapters/notebook/notebook.ts

@@ -61,6 +61,7 @@ export class NotebookAdapter extends WidgetAdapter<NotebookPanel> {
     if (this.isDisposed) {
       return;
     }
+
     this.widget.context.sessionContext.kernelChanged.disconnect(
       this.on_kernel_changed,
       this
@@ -69,8 +70,11 @@ export class NotebookAdapter extends WidgetAdapter<NotebookPanel> {
       this.activeCellChanged,
       this
     );
-    this.ce_editor_to_cell.clear();
+
     super.dispose();
+
+    // editors are needed for the parent dispose() to unbind signals, so they are the last to go
+    this.ce_editor_to_cell.clear();
   }
 
   is_ready = () => {

packages/jupyterlab-lsp/src/virtual/document.spec.ts

@@ -3,6 +3,7 @@ import { is_within_range, VirtualDocument } from './document';
 import { ISourcePosition, IVirtualPosition } from '../positioning';
 import { CodeEditor } from '@jupyterlab/codeeditor';
 import { foreign_code_extractors } from '../extractors/defaults';
+import Mock = jest.Mock;
 
 let R_LINE_MAGICS = `%R df = data.frame()
 print("df created")
@@ -45,14 +46,34 @@ describe('is_within_range', () => {
 });
 
 describe('VirtualDocument', () => {
-  let document = new VirtualDocument({
-    language: 'python',
-    path: 'test.ipynb',
-    overrides_registry: {},
-    foreign_code_extractors: foreign_code_extractors,
-    standalone: false,
-    file_extension: 'py',
-    has_lsp_supported_file: false
+  let document: VirtualDocument;
+  beforeEach(() => {
+    document = new VirtualDocument({
+      language: 'python',
+      path: 'test.ipynb',
+      overrides_registry: {},
+      foreign_code_extractors: foreign_code_extractors,
+      standalone: false,
+      file_extension: 'py',
+      has_lsp_supported_file: false
+    });
+  });
+
+  describe('#dispose', () => {
+    it('disposes, but does not break methods which can be called from async callbacks', () => {
+      expect(document.isDisposed).to.equal(false);
+      // appending code block here should work fine
+      document.append_code_block('code', {} as CodeEditor.IEditor);
+      document.dispose();
+      expect(document.isDisposed).to.equal(true);
+      // mock console.warn
+      console.warn = jest.fn();
+      // this one should not raise, but just warn
+      document.append_code_block('code', {} as CodeEditor.IEditor);
+      expect((console.warn as Mock).mock.calls[0][0]).to.equal(
+        'Cannot append code block: document disposed'
+      );
+    });
   });
 
   describe('#extract_foreign_code', () => {
@@ -113,7 +134,7 @@ describe('VirtualDocument', () => {
     );
   };
 
-  describe('transform_virtual_to_editor', () => {
+  describe('#transform_virtual_to_editor', () => {
     it('transforms positions for the top level document', () => {
       init_document_with_Python_and_R();
       // The first (Python) line in the first block

packages/jupyterlab-lsp/src/virtual/document.ts

@@ -251,6 +251,7 @@ export class VirtualDocument {
     if (this.isDisposed) {
       return;
     }
+    this.isDisposed = true;
 
     this.parent = null;
 
@@ -276,9 +277,6 @@ export class VirtualDocument {
     this.line_magics_overrides = null;
     this.lines = null;
     this.overrides_registry = null;
-
-    // actually disposed now
-    this.isDisposed = true;
   }
 
   /**
@@ -595,6 +593,10 @@ export class VirtualDocument {
     editor_shift: CodeEditor.IPosition = { line: 0, column: 0 },
     virtual_shift?: CodeEditor.IPosition
   ) {
+    if (this.isDisposed) {
+      console.warn('Cannot append code block: document disposed');
+      return;
+    }
     let source_cell_lines = cell_code.split('\n');
 
     let { lines, foreign_document_map, skip_inspect } = this.prepare_code_block(