From 82f16e25fbdc969fdfc5ca9b208efa25a299baf0 Mon Sep 17 00:00:00 2001 From: TylerZeroMaster <33585550+TylerZeroMaster@users.noreply.github.com> Date: Tue, 21 May 2024 10:45:16 -0500 Subject: [PATCH] Mark nodes removed; only remove orphaned nodes (#195) * Mark nodes removed instead of deleting them Problem: - When POET loads a page, references to resources on the page are stored in the page node (see _resourceLinks) - When a resource was removed from the bundle, perhaps via a filesystem event, the references in the page remained, even though the resources no longer existed in the bundle. - When a resource with the same absPath was created, it was added to the bundle again. This new resource instance, however, was distinct from the one that existed within the page even though they pointed to the same content on the filesystem. - Until the page was updated, which causes the links to be reconstructed, resource links stored in the page did not correlate with the ones stored in the bundle. Consequently, the `exists` property of the resource node was not being updated Solution: - Keep all nodes in the bundle - When a file referenced by a node is deleted from the filesystem, update the node's contents to `undefined` so that the node's `exists` property will be set to false Possible problems & justifications: - This technically introduces a memory leak where nodes continue to accumulate in the bundle forever; however, there are many ways this memory leak could be addressed if it becomes a real problem. - Out of all the approaches I explored, this one made the best use of Quarx, required the least number of changes, and exhibited the best performance. * Remove deleted nodes that are orphaned This fixes the aforementioned memory leak for everything except bundles When a file is deleted and its associated node is orphaned, as defined in model manager, then that node will be removed If the node is not orphaned, it will stay in memory even though the file no longer exists * Fix some comments; add some polish * Send diagnostics before removing nodes * Use readAndUpdate instead of readAndLoad A file creation event could map to an existing node that is loaded * Nodes only need to be loaded to be orphaned Fixes a problem with cascading deletions --- server/src/model-manager.spec.ts | 30 ++++++++- server/src/model-manager.ts | 104 ++++++++++++++++++++----------- server/src/model/bundle.ts | 26 +++++--- server/src/model/factory.spec.ts | 8 +-- server/src/model/factory.ts | 7 +-- server/src/model/resource.ts | 2 +- server/src/model/utils.ts | 2 +- server/src/server-handler.ts | 2 +- server/src/server.ts | 2 +- 9 files changed, 125 insertions(+), 58 deletions(-) diff --git a/server/src/model-manager.spec.ts b/server/src/model-manager.spec.ts index 800af9bc..384c3cec 100644 --- a/server/src/model-manager.spec.ts +++ b/server/src/model-manager.spec.ts @@ -317,18 +317,46 @@ describe('processFilesystemChange()', () => { // Load the Bundle, Book, and Page loadSuccess(first(loadSuccess(first(loadSuccess(manager.bundle).books)).pages)) + const sizeBefore = manager.bundle.allNodes.size + expect(manager.bundle.allPages.all.size).toBe(1) + expect(manager.bundle.allResources.all.size).toBe(0) // Delete non-existent file expect((await fireChange(FileChangeType.Deleted, 'media/newpic.png')).toArray()).toEqual([]) // Delete a file const deletedModules = await fireChange(FileChangeType.Deleted, 'modules/m1234/index.cnxml') expect(deletedModules.size).toBe(1) expect(first(deletedModules)).toBeInstanceOf(PageNode) + // Page is still referenced by book: should still be in bundle + expect(manager.bundle.allPages.all.size).toBe(1) // Delete a directory expect((await fireChange(FileChangeType.Deleted, 'collections')).size).toBe(1) // expect(sendDiagnosticsStub.callCount).toBe(0) + // Book deleted -> page orphaned -> page deleted + expect(manager.bundle.allPages.all.size).toBe(0) // Delete everything (including the bundle) - expect((await fireChange(FileChangeType.Deleted, '')).size).toBe(1) + // All nodes should still be in bundle, but they should no longer exist + expect((await fireChange(FileChangeType.Deleted, '')).size).toBe(sizeBefore - 1) + expect(manager.bundle.exists).toBe(false) + expect(manager.bundle.allNodes.every((n) => !n.exists)) + }) + it('cascades deletes correctly', async () => { + // Load the Bundle, Book, and Page + loadSuccess(first(loadSuccess(first(loadSuccess(manager.bundle).books)).pages)) + + const sizeBefore = manager.bundle.allNodes.size + expect(manager.bundle.allBooks.all.size).toBe(1) + expect(manager.bundle.allPages.all.size).toBe(1) + expect(manager.bundle.allResources.all.size).toBe(0) + expect(manager.bundle.allNodes.size).toBe(3) // bundle + book + page + + // Delete everything (including the bundle) + expect((await fireChange(FileChangeType.Deleted, '')).size).toBe(sizeBefore) + expect(manager.bundle.allBooks.all.size).toBe(0) + expect(manager.bundle.allPages.all.size).toBe(0) + // Only the bundle should remain (bundles are immortal at the moment) + expect(manager.bundle.allNodes.size).toBe(1) + expect(first(manager.bundle.allNodes)).toBe(manager.bundle) expect(manager.bundle.exists).toBe(false) }) it('deletes Image/Page/Book', async () => { diff --git a/server/src/model-manager.ts b/server/src/model-manager.ts index d9e41b51..ea2694ff 100644 --- a/server/src/model-manager.ts +++ b/server/src/model-manager.ts @@ -175,21 +175,36 @@ export class ModelManager { return this.bundle.allPages.all } + public get orphanedBooks() { + const loadedBooks = this.bundle.allBooks.all.filter((n) => n.isLoaded) + const referencedBooks = this.bundle.books + return !this.bundle.exists ? loadedBooks : loadedBooks.subtract(referencedBooks) + } + public get orphanedPages() { const books = this.bundle.books.filter(loadedAndExists) - return this.bundle.allPages.all.filter(loadedAndExists).subtract(books.flatMap(b => b.pages)) + return this.bundle.allPages.all.filter((n) => n.isLoaded).subtract(books.flatMap(b => b.pages)) } public get orphanedResources() { const books = this.bundle.books.filter(loadedAndExists) - const pages = books.flatMap(b => b.pages) - return this.bundle.allResources.all.filter(loadedAndExists).subtract(pages.flatMap(p => p.resources)) + const pages = books.flatMap(b => b.pages).filter(loadedAndExists) + return this.bundle.allResources.all.filter((n) => n.isLoaded).subtract(pages.flatMap(p => p.resources)) } public get orphanedH5P() { const books = this.bundle.books.filter(loadedAndExists) - const pages = books.flatMap(b => b.pages) - return this.bundle.allH5P.all.filter(loadedAndExists).subtract(pages.flatMap(p => p.h5p)) + const pages = books.flatMap(b => b.pages).filter(loadedAndExists) + return this.bundle.allH5P.all.filter((n) => n.isLoaded).subtract(pages.flatMap(p => p.h5p)) + } + + public get orphanedNodes() { + return I.Set().withMutations((s) => { + s.union(this.orphanedBooks) + s.union(this.orphanedPages) + s.union(this.orphanedH5P) + s.union(this.orphanedResources) + }) } public async loadEnoughForToc() { @@ -246,7 +261,7 @@ export class ModelManager { const node = findOrCreateNode(bundle, uri) if (node !== undefined) { ModelManager.debug('[FILESYSTEM_EVENT] Adding item') - await this.readAndLoad(node) + await this.readAndUpdate(node) this.sendAllDiagnostics() return I.Set([node]) } else { @@ -270,28 +285,42 @@ export class ModelManager { ModelManager.debug('[FILESYSTEM_EVENT] Removing everything with this URI (including subdirectories if they exist)', uri) const removedNodes = I.Set().withMutations(s => { - // Unload if the user deleted the bundle directory - if (bundle.absPath.startsWith(uri)) s.add(bundle) - // Remove if it was a file - const removedNode = bundle.allBooks.remove(uri) ?? - bundle.allPages.remove(uri) ?? - bundle.allResources.remove(uri) ?? - bundle.allH5P.remove(uri) - if (removedNode !== undefined) s.add(removedNode) - // Remove if it was a directory + const markRemoved = (n: T) => { + ModelManager.debug(`[MODEL_MANAGER] Marking as removed: ${n.absPath}`) + this.errorHashesByPath.delete(n.absPath) + n.load(undefined) + s.add(n) + } const filePathDir = `${uri}${PATH_SEP}` - s.union(bundle.allBooks.removeByKeyPrefix(filePathDir)) - s.union(bundle.allPages.removeByKeyPrefix(filePathDir)) - s.union(bundle.allResources.removeByKeyPrefix(filePathDir)) - s.union(bundle.allH5P.removeByKeyPrefix(filePathDir)) - }) - // Unload all removed nodes so users do not think the files still exist - removedNodes.forEach(n => { - this.errorHashesByPath.delete(n.absPath) - n.load(undefined) - this.sendFileDiagnostics(n) + // NOTE: order is important. Ideally try to delete things that could + // hold references first (books/pages) + const allFactories = [ + bundle.allBooks, bundle.allPages, bundle.allH5P, bundle.allResources + ] + + // First mark all the matching nodes as mark not existing (i.e. load undefined) + if (bundle.absPath.startsWith(uri)) markRemoved(bundle) + allFactories.forEach((factory) => { + const maybeNode = factory.get(uri) + if (maybeNode !== undefined) markRemoved(maybeNode) + factory.findByKeyPrefix(filePathDir).forEach(markRemoved) + }) + + // Send diagnostics before removing nodes + this.sendAllDiagnostics() + + // Then remove nodes if they are orphaned, loaded, and not existing + this.orphanedNodes + .filter(({ isLoaded, exists }) => isLoaded && !exists) + .forEach(({ absPath }) => { + ModelManager.debug(`[MODEL_MANAGER] Dropping: ${absPath}`) + // Expect at least one factory to have this node + expectValue( + allFactories.find((factory) => factory.remove(absPath) !== undefined), + `[MODEL_MANAGER] ERROR: Failed to drop: ${absPath} (possible memory leak)` + ) + }) }) - this.sendAllDiagnostics() return removedNodes } } @@ -448,14 +477,17 @@ export class ModelManager { .length > 0 }, getRange: this.rangeFinderFactory('src="', '"'), - getCompletionItems: (page, range) => this.orphanedResources.toArray().map(i => { - const insertText = path.relative(path.dirname(page.absPath), i.absPath) - const item = CompletionItem.create(insertText) - item.textEdit = TextEdit.replace(range, insertText) - item.kind = CompletionItemKind.File - item.detail = 'Orphaned Resource' - return item - }) + getCompletionItems: (page, range) => this.orphanedResources + .filter((r) => r.exists) + .toArray() + .map(i => { + const insertText = path.relative(path.dirname(page.absPath), i.absPath) + const item = CompletionItem.create(insertText) + item.textEdit = TextEdit.replace(range, insertText) + item.kind = CompletionItemKind.File + item.detail = 'Orphaned Resource' + return item + }) } return this.autocomplete(page, cursor, resourceAutocompleter) } @@ -470,7 +502,9 @@ export class ModelManager { }, getRange: this.rangeFinderFactory('url="', '"'), getCompletionItems: (_page, range) => { - return this.orphanedH5P.toArray().filter((h) => h.exists) + return this.orphanedH5P + .filter((h) => h.exists) + .toArray() .map((h) => path.dirname(h.absPath)) .map((p) => path.basename(p)) .map((name) => { diff --git a/server/src/model/bundle.ts b/server/src/model/bundle.ts index 37d7b2fe..dc50a0b9 100644 --- a/server/src/model/bundle.ts +++ b/server/src/model/bundle.ts @@ -14,7 +14,7 @@ export class Bundle extends Fileish implements Bundleish { public readonly allH5P: Factory = new Factory((absPath: string) => new H5PExercise(this, this.pathHelper, absPath), (x) => this.pathHelper.canonicalize(x)) public readonly allBooks = new Factory((absPath: string) => new BookNode(this, this.pathHelper, absPath), (x) => this.pathHelper.canonicalize(x)) private readonly _books = Quarx.observable.box>>>(undefined) - private readonly _duplicateResourcePaths = Quarx.observable.box>(I.Set()) + private readonly _duplicateFilePaths = Quarx.observable.box>(I.Set()) private readonly _duplicateUUIDs = Quarx.observable.box>(I.Set()) // TODO: parse these from META-INF/books.xml public readonly paths = { @@ -29,10 +29,12 @@ export class Bundle extends Fileish implements Bundleish { super(undefined, pathHelper, pathHelper.join(workspaceRootUri, 'META-INF/books.xml')) super.setBundle(this) Quarx.autorun(() => { - this._duplicateResourcePaths.set( + this._duplicateFilePaths.set( I.Set( - findDuplicates(I.List(this.allResources.all) - .map(n => n.absPath.toLowerCase()) + findDuplicates( + I.List(this.allNodes) + .filter(n => n.exists) + .map(n => n.absPath.toLowerCase()) ) ) ) @@ -40,9 +42,8 @@ export class Bundle extends Fileish implements Bundleish { Quarx.autorun(() => { this._duplicateUUIDs.set( I.Set( - findDuplicates(I.List(this.allPages.all) - .filter(n => n.exists) - .map(n => n.uuid()) + findDuplicates( + I.List(this.allPages.all).filter(n => n.exists).map(n => n.uuid()) ) ) ) @@ -63,15 +64,20 @@ export class Bundle extends Fileish implements Bundleish { } public get allNodes() { - return I.Set([this]).union(this.allBooks.all).union(this.allPages.all).union(this.allResources.all).union(this.allH5P.all) + // TODO: Will all nodes continue to be fileish in future? + return I.Set([this]) + .union(this.allBooks.all) + .union(this.allPages.all) + .union(this.allH5P.all) + .union(this.allResources.all) } public get books() { return this.__books().map(b => b.v) } - public isDuplicateResourcePath(path: string): boolean { - return this._duplicateResourcePaths.get().has(path.toLowerCase()) + public isDuplicateFilePath(path: string): boolean { + return this._duplicateFilePaths.get().has(path.toLowerCase()) } private __books() { diff --git a/server/src/model/factory.spec.ts b/server/src/model/factory.spec.ts index d78f02c6..dca27159 100644 --- a/server/src/model/factory.spec.ts +++ b/server/src/model/factory.spec.ts @@ -14,7 +14,7 @@ describe('Factory', () => { expect(f.getOrAdd('key2').thing).toEqual(1) expect(f.get('key2')).not.toBeUndefined() }) - it('removesByKeyPrefix works', () => { + it('findByKeyPrefix works', () => { const f = new Factory((x) => ({ foo: x, bar: 'dummy-object' }), (x) => x) f.getOrAdd('keyPrefix1') f.getOrAdd('keyPrefix2') @@ -22,8 +22,8 @@ describe('Factory', () => { f.getOrAdd('not_a_keyPrefix') expect(f.size).toEqual(4) - const removed = f.removeByKeyPrefix('keyPrefix') - expect(removed.size).toEqual(2) - expect(f.size).toEqual(2) + const found = f.findByKeyPrefix('keyPrefix') + expect(found.size).toEqual(2) + expect(f.size).toEqual(4) }) }) diff --git a/server/src/model/factory.ts b/server/src/model/factory.ts index 80250aee..8fa09acd 100644 --- a/server/src/model/factory.ts +++ b/server/src/model/factory.ts @@ -30,12 +30,11 @@ export class Factory { return item } - public removeByKeyPrefix(pathPrefix: string) { + public findByKeyPrefix(pathPrefix: string) { pathPrefix = this.canonicalizer(pathPrefix) const m = this._map.get() - const removedItems = m.filter((_, key) => key.startsWith(pathPrefix)) - this._map.set(m.filter((_, key) => !key.startsWith(pathPrefix))) - return I.Set(removedItems.values()) + const matchingItems = m.filter((_, key) => key.startsWith(pathPrefix)) + return I.Set(matchingItems.values()) } public get size() { return this._map.get().size } diff --git a/server/src/model/resource.ts b/server/src/model/resource.ts index de9e725f..2b8db715 100644 --- a/server/src/model/resource.ts +++ b/server/src/model/resource.ts @@ -8,7 +8,7 @@ export class ResourceNode extends Fileish { return [{ message: ResourceValidationKind.DUPLICATE_RESOURCES, nodesToLoad: I.Set(), - fn: () => this.bundle.isDuplicateResourcePath(this.absPath) + fn: () => this.bundle.isDuplicateFilePath(this.absPath) ? I.Set([NOWHERE]) : I.Set() }] diff --git a/server/src/model/utils.ts b/server/src/model/utils.ts index caeaf19e..6620a63c 100644 --- a/server/src/model/utils.ts +++ b/server/src/model/utils.ts @@ -81,7 +81,7 @@ export interface Bundleish { allH5P: Factory workspaceRootUri: string isDuplicateUuid: (uuid: string) => boolean - isDuplicateResourcePath: (path: string) => boolean + isDuplicateFilePath: (path: string) => boolean paths: Paths } diff --git a/server/src/server-handler.ts b/server/src/server-handler.ts index 543d00f4..141f5ecc 100644 --- a/server/src/server-handler.ts +++ b/server/src/server-handler.ts @@ -50,7 +50,7 @@ export async function autocompleteHandler(documentPosition: CompletionParams, ma const cursor = documentPosition.position const page = manager.bundle.allPages.get(documentPosition.textDocument.uri) - if (page !== undefined) { + if (page !== undefined && page.exists) { return [ ...manager.autocompleteResources(page, cursor), ...manager.autocompleteUrls(page, cursor) diff --git a/server/src/server.ts b/server/src/server.ts index 811f6485..1bc32f37 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -166,7 +166,7 @@ connection.onDocumentLinks(async ({ textDocument }) => { const { uri } = textDocument const manager = getBundleForUri(uri) const page = manager.bundle.allPages.get(uri) - if (page !== undefined) { + if (page !== undefined && page.exists) { return await manager.getDocumentLinks(page) } else { return []