diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index 6a69eeb34f..2332c3af8f 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -263,14 +263,17 @@ class Entry { return result } - /** Let a write from another entry go next: `turn` resolves once the commands queued so far have - * run, and any queued after wait for `release`. */ - hold(): { turn: Promise; release: () => void } { + /** Let a move onto this key go next: resolves once the commands queued so far have run, and + * holds any queued after until `until`. */ + hold(until: Promise): Promise { const turn = this.queue - let release!: () => void - const released = new Promise((r) => (release = r)) - this.queue = turn.then(() => released) - return { turn, release } + this.queue = turn.then(() => until) + return turn + } + + /** A new entry at a key a move is heading for: its commands, its first load included, wait. */ + startAfter(move: Promise): void { + this.queue = move } /** Count a command as started now, ahead of its turn in the queue; returns its release. */ @@ -297,6 +300,7 @@ class Entry { const key = this.key let res: ItemLoad try { + if (this.retired) return superseded if (!adapter.load) throw new Error('This item cannot be loaded') res = await adapter.load(key) } catch (e) { @@ -394,12 +398,12 @@ class Entry { this.origin = 'deployed' this.template = undefined if (moved) this.moveTo(to) + this.reconcile() + await this.settleRows(moved ? [from, this.key] : [this.key]) + return { ok: true, path: to, moved } } finally { claim?.release() } - this.reconcile() - await this.settleRows(moved ? [from, this.key] : [this.key]) - return { ok: true, path: to, moved } } if (!started) return this.run(body) const result = this.queue.then(body).finally(started) @@ -727,6 +731,8 @@ export type ItemAcquisition = { handle: ItemHandle; release(): void } export function createItemStore(ports: ItemRowPort) { const entries = new Map>() + /** Saves moving onto a key, by that key: each settles once its move is done. */ + const moves = new Map>() const internals: StoreInternals = { release(entry) { @@ -747,20 +753,28 @@ export function createItemStore(ports: ItemRowPort) { entries.set(to, entry) }, /** - * A save about to write the item at `key` and move onto it, while another live entry holds - * that key: it goes after the commands already queued there, and holds back any queued - * meanwhile, so one write to the item lands at a time and the displaced entry is idle when - * retired. Not when the holder is itself waiting to move onto the claimant: each would wait - * for the other. + * A save about to write the item at `key` and move onto it. It goes after whatever is queued + * at that key (the entry holding it, or a move already heading there), and until it is done + * everything else at that key waits: that holder's later commands, an entry acquired there + * meanwhile, another move. So one write to the item lands at a time, and whatever the move + * retires is idle by then. Not when the holder is itself waiting to move onto the claimant: + * each would wait for the other. */ claim(key, claimant) { - const holder = entries.get(keyString(key)) - if (!holder || holder === claimant || waitsFor(holder, claimant)) return undefined - const { turn, release } = holder.hold() + const k = keyString(key) + const holder = entries.get(k) + if (holder === claimant || (holder && waitsFor(holder, claimant))) return undefined + let release!: () => void + const released = new Promise((r) => (release = r)) + const turn = holder ? holder.hold(released) : (moves.get(k) ?? Promise.resolve()) + moves.set(k, released) claimant.awaiting = key return { turn: turn.then(() => (claimant.awaiting = undefined)), - release + release() { + release() + if (moves.get(k) === released) moves.delete(k) + } } } } @@ -821,6 +835,8 @@ export function createItemStore(ports: ItemRowPort) { entry = new Entry(key, ports, internals) entry.settles = adapter.settles ?? false entry.absorbs = adapter.absorbs + const move = moves.get(k) + if (move) entry.startAfter(move) entries.set(k, entry) if (isTemporaryPath(key.path) && spec.template !== undefined) entry.initNew(spec.template) else void entry.load(adapter) diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 8870f840b9..eb04b35eea 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -539,6 +539,48 @@ describe('item store: one entry per key', () => { expect(order).toEqual(['first', 'second']) expect(other.deployed?.description).toBe('second') }) + + it('holds an editor that opens an item while a save is moving onto it', async () => { + const rows = fakeRows() + const store = createItemStore(rows.port) + const order: string[] = [] + const gate = deferred() + const b = { ...deployedRes, path: 'u/me/b' } + const temporary = newItemPath() + const { handle: moving } = store.acquire( + { workspace: 'w', kind: 'resource', path: temporary }, + { workspace: 'w', path: temporary, template: b }, + adapter({}, async (ctx) => { + await gate.promise + order.push(`moved: ${ctx.value.description}`) + }) + ) + await settle() + moving.value = { ...b, description: 'moving' } + const moved = moving.save() + await settle() + + const { handle: opened } = store.acquire( + { workspace: 'w', kind: 'resource', path: 'u/me/b' }, + { workspace: 'w', path: 'u/me/b' }, + adapter( + async () => { + order.push('loaded') + return { deployed: b } + }, + async (ctx) => void order.push(`saved: ${ctx.value.description}`) + ) + ) + const savedMeanwhile = opened.save() + await settle() + expect(order).toEqual([]) + + gate.resolve() + expect(await moved).toMatchObject({ ok: true, moved: true }) + expect(await savedMeanwhile).toMatchObject({ ok: false }) + expect(order).toEqual(['moved: moving']) + expect(opened.value?.description).toBe('moving') + }) }) describe('item store: conflicts', () => {