From 6e3deca87ddee457d158129674dd591e58a61a98 Mon Sep 17 00:00:00 2001 From: Danny Hoek Date: Fri, 14 Aug 2026 21:15:36 +0200 Subject: [PATCH 1/3] fix tile cancellation in the image data worker The proxied onAbort callback was invoked at the top of getImageData, so every tile aborted its own controller the instant its fetch began, while the worker's controller was never aborted at all. Each proxy also opened a MessageChannel per tile. Replace it with an abort method keyed by tile url, cancel through the decode and the pixel copy rather than only the download, and close the decoded bitmap on both exits. Track in-flight tiles in a set of counted urls, so a tile that already reported back is not counted out a second time when it is later pruned. That kept tilesFetchingCount from going negative, which would announce that all tiles are loaded while one is still in flight. --- .../tilecache/CacheableWorkerImageDataTile.ts | 32 +- packages/render/src/tilecache/TileCache.ts | 62 ++- .../src/workers/fetch-and-get-image-data.ts | 58 ++- .../test/fetch-and-get-image-data.test.ts | 467 ++++++++++++++++++ packages/render/test/tile-cache.test.ts | 153 ++++++ 5 files changed, 722 insertions(+), 50 deletions(-) create mode 100644 packages/render/test/fetch-and-get-image-data.test.ts create mode 100644 packages/render/test/tile-cache.test.ts diff --git a/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts b/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts index d962fd16c..333265f18 100644 --- a/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts +++ b/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts @@ -1,4 +1,4 @@ -import { proxy as comlinkProxy, type Remote as ComlinkRemote } from 'comlink' +import { type Remote as ComlinkRemote } from 'comlink' import { FetchableTile } from './FetchableTile.js' import { CacheableTile, CachedTile } from './CacheableTile.js' @@ -18,6 +18,7 @@ import type { ApplySpritesImageDataWorkerType } from '../workers/apply-sprites-i export class CacheableWorkerImageDataTile extends CacheableTile { #workerPool: WorkerPool #spritesWorker: ComlinkRemote + #fetchingWorker: ComlinkRemote | null = null constructor( fetchableTile: FetchableTile, @@ -37,16 +38,20 @@ export class CacheableWorkerImageDataTile extends CacheableTile { */ async fetch() { const { worker, index } = this.#workerPool.acquire() + this.#fetchingWorker = worker try { worker .getImageData( this.fetchableTile.tileUrl, - comlinkProxy(() => this.abortController.abort()), this.fetchFn, this.fetchableTile.tile.tileZoomLevel.width, this.fetchableTile.tile.tileZoomLevel.height ) .then((response) => { + if (this.abortController.signal.aborted) { + return + } + this.data = response this.dispatchEvent( new WarpedMapEvent(WarpedMapEventType.TILEFETCHED, { @@ -55,16 +60,18 @@ export class CacheableWorkerImageDataTile extends CacheableTile { ) }) .catch((err) => { - if (err instanceof Error && err.name === 'AbortError') { - console.log('Fetch aborted') // Handle the abort error - } else { - console.error(err) // Handle other errors + if (!(err instanceof Error && err.name === 'AbortError')) { + // TODO: comlink keeps only message/name/stack, so a + // ResourceFetchError arrives stripped and reports as 'unknown'. + this.dispatchTileFetchError(err) } }) .finally(() => { + this.#fetchingWorker = null this.#workerPool.release(index) }) } catch (err) { + this.#fetchingWorker = null this.#workerPool.release(index) // release even if setup itself throws synchronously if (err instanceof Error && err.name === 'AbortError') { // fetchImage was aborted because viewport was moved and tile @@ -77,6 +84,19 @@ export class CacheableWorkerImageDataTile extends CacheableTile { return this.data } + /** The worker cannot see our AbortSignal, so it has to be told separately. */ + override abort() { + if (this.abortController.signal.aborted) { + return + } + + super.abort() + // Nobody awaits this reply; swallow so a rejection is not left unhandled. + this.#fetchingWorker + ?.abort(this.fetchableTile.tileUrl) + .catch(() => undefined) + } + async applySprites() { const data = this.data const spritesInfo = this.fetchableTile.options?.spritesInfo diff --git a/packages/render/src/tilecache/TileCache.ts b/packages/render/src/tilecache/TileCache.ts index 758e32fda..480c398a8 100644 --- a/packages/render/src/tilecache/TileCache.ts +++ b/packages/render/src/tilecache/TileCache.ts @@ -34,7 +34,6 @@ export class TileCache extends EventTarget { protected mapIdsByTileUrl: Map> = new Map() protected tileUrlsByMapId: Map> = new Map() - protected tilesFetchingCount = 0 protected tileRemoveQueue: { tileUrl: string mapId: string @@ -42,6 +41,13 @@ export class TileCache extends EventTarget { protected fetchableTiles: FetchableTile[] = [] + /** + * The tiles in flight. Deleting returns true only for the first caller, so a + * tile is counted out exactly once no matter how it ends: fetched, failed, or + * removed while still fetching. + */ + #tilesFetching: Set = new Set() + #boundTileFetched = this.tileFetched.bind(this) #boundTileFetchError = this.tileFetchError.bind(this) #boundTilesFromSpriteTile = this.tilesFromSpriteTile.bind(this) @@ -211,8 +217,8 @@ export class TileCache extends EventTarget { /** * Returns a promise that resolves when all requested tiles are loaded. - * This could happen immidiately, in case there are no ongoing requests and the tilesFetchingCount is zero, - * or in a while, when the count reaches zero and the ALLREQUESTEDTILESLOADED event is fired. + * This could happen immidiately, in case there are no ongoing requests, + * or in a while, when the last one finishes and ALLREQUESTEDTILESLOADED is fired. */ async allRequestedTilesLoaded(): Promise { return new Promise((resolve) => { @@ -268,7 +274,7 @@ export class TileCache extends EventTarget { this.tilesByTileUrl = new Map() this.mapIdsByTileUrl = new Map() this.tileUrlsByMapId = new Map() - this.tilesFetchingCount = 0 + this.#tilesFetching = new Set() } destroy() { @@ -310,7 +316,7 @@ export class TileCache extends EventTarget { // This is an async function that we are not awaiting to continue // The results are handled inside the tile using events cacheableTile.fetch() - this.updateTilesFetchingCount(1) + this.startFetching(cacheableTile.fetchableTile.tileUrl) } // Directly add cached tiles created from sprites @@ -335,6 +341,14 @@ export class TileCache extends EventTarget { } protected delayedRemoveCacheableTileForMapId(tileUrl: string, mapId: string) { + const cacheableTile = this.tilesByTileUrl.get(tileUrl) + + // No pixels yet to keep, so queueing only pays for the rest of a download. + if (cacheableTile && !cacheableTile.isCachedTile()) { + this.removeCacheableTileForMapId(tileUrl, mapId) + return + } + if ( this.tileRemoveQueue.some( (tile) => tile.tileUrl === tileUrl && tile.mapId === mapId @@ -375,15 +389,14 @@ export class TileCache extends EventTarget { const mapIds = this.removeMapIdForTileUrl(mapId, tileUrl) this.removeTileUrlForMapId(tileUrl, mapId) - // If there are no other maps for this tile and it's still fetching, - // abort the fetch and delete the tile from the cache. + // No other map wants this tile, so it leaves the cache. if (!mapIds.size) { - if (!cacheableTile.isCachedTile()) { - // Cancel fetch if tile is still being fetched + // Still fetching means the download is live, so stop it. + if (this.stopFetching(tileUrl)) { cacheableTile.abort() - this.updateTilesFetchingCount(-1) } + this.removeEventListenersFromCacheableTile(cacheableTile) this.tilesByTileUrl.delete(tileUrl) } @@ -402,7 +415,7 @@ export class TileCache extends EventTarget { } const { tileUrl } = event.data - this.updateTilesFetchingCount(-1) + this.stopFetching(tileUrl) for (const mapId of this.mapIdsByTileUrl.get(tileUrl) || []) { this.dispatchEvent( @@ -439,11 +452,7 @@ export class TileCache extends EventTarget { } const { tileUrl } = event.data - // A failed fetch must decrement the in-flight count just like a successful - // one (see tileFetched), otherwise tilesFetchingCount never reaches zero. - if (this.tilesByTileUrl.has(tileUrl)) { - this.updateTilesFetchingCount(-1) - } + this.stopFetching(tileUrl) const mapIds = [ ...new Set([ @@ -577,24 +586,33 @@ export class TileCache extends EventTarget { } get finished() { - return this.tilesFetchingCount === 0 + return this.#tilesFetching.size === 0 } - protected updateTilesFetchingCount(delta: number) { - const previousTilesFetchingCount = this.tilesFetchingCount - this.tilesFetchingCount += delta + protected startFetching(tileUrl: string) { + const previousCount = this.#tilesFetching.size + this.#tilesFetching.add(tileUrl) - if (previousTilesFetchingCount === 0 && this.tilesFetchingCount > 0) { + if (previousCount === 0 && this.#tilesFetching.size > 0) { this.dispatchEvent( new WarpedMapEvent(WarpedMapEventType.REQUESTEDTILESLOADING) ) } + } - if (this.tilesFetchingCount === 0) { + /** False if it had already stopped, so each tile is counted out once. */ + protected stopFetching(tileUrl: string) { + if (!this.#tilesFetching.delete(tileUrl)) { + return false + } + + if (this.#tilesFetching.size === 0) { this.dispatchEvent( new WarpedMapEvent(WarpedMapEventType.ALLREQUESTEDTILESLOADED) ) } + + return true } protected addEventListenersToCacheableTile(cacheableTile: CacheableTile) { diff --git a/packages/render/src/workers/fetch-and-get-image-data.ts b/packages/render/src/workers/fetch-and-get-image-data.ts index 779a0636d..66b1c5d30 100644 --- a/packages/render/src/workers/fetch-and-get-image-data.ts +++ b/packages/render/src/workers/fetch-and-get-image-data.ts @@ -4,42 +4,56 @@ import { fetchUrl } from '@allmaps/stdlib' import type { FetchFn } from '@allmaps/types' -const fetchAndGetImageDataWorker = { +export const abortControllers = new Map() + +export const fetchAndGetImageDataWorker = { async getImageData( tileUrl: string, - onAbort: () => void, // Define as a no-arguments function fetchFn: FetchFn | undefined, width: number, height: number ): Promise { - const workerAbortController = new AbortController() + const abortController = new AbortController() + const { signal } = abortController + abortControllers.set(tileUrl, abortController) - // Connect the abort signal with a listener - onAbort() + try { + const response = await fetchUrl(tileUrl, { signal }, fetchFn) - const response = await fetchUrl( - tileUrl, - { - signal: workerAbortController.signal - }, - fetchFn - ) + const blob = await response.blob() + signal.throwIfAborted() - const blob = await response.blob() + const imageBitmap = await createImageBitmap(blob, 0, 0, width, height) - const imageBitmap = await createImageBitmap(blob, 0, 0, width, height) + try { + signal.throwIfAborted() - const canvas = new OffscreenCanvas(width, height) - const context = canvas.getContext('2d') + const canvas = new OffscreenCanvas(width, height) + const context = canvas.getContext('2d') - if (!context) { - throw new Error('Could not create OffscreenCanvas context') - } + if (!context) { + throw new Error('Could not create OffscreenCanvas context') + } - context.drawImage(imageBitmap, 0, 0) - const imageData = context.getImageData(0, 0, width, height) + context.drawImage(imageBitmap, 0, 0) + const imageData = context.getImageData(0, 0, width, height) + + return transfer(imageData, [imageData.data.buffer]) + } finally { + imageBitmap.close() + } + } finally { + // A later fetch for this url may own the entry now; deleting it then + // would leave that fetch unabortable. + if (abortControllers.get(tileUrl) === abortController) { + abortControllers.delete(tileUrl) + } + } + }, - return transfer(imageData, [imageData.data.buffer]) + /** Runs while getImageData is still awaiting: the worker is idle on I/O. */ + abort(tileUrl: string): void { + abortControllers.get(tileUrl)?.abort() } } diff --git a/packages/render/test/fetch-and-get-image-data.test.ts b/packages/render/test/fetch-and-get-image-data.test.ts new file mode 100644 index 000000000..010e65965 --- /dev/null +++ b/packages/render/test/fetch-and-get-image-data.test.ts @@ -0,0 +1,467 @@ +import { afterEach, describe, expect, test, vi } from 'vitest' +import { expose, wrap } from 'comlink' + +import { CacheableWorkerImageDataTile } from '../src/tilecache/CacheableWorkerImageDataTile.js' +import { WarpedMapEventType } from '../src/shared/events.js' +import { FetchableTile } from '../src/tilecache/FetchableTile.js' + +import type { Tile } from '@allmaps/types' + +import type { WorkerPool } from '../src/workers/PoolWorkers.js' +import type { FetchAndGetImageDataWorkerType } from '../src/workers/fetch-and-get-image-data.js' +import type { ApplySpritesImageDataWorkerType } from '../src/workers/apply-sprites-image-data.js' + +const TILE_URL = 'https://example.com/tile.jpg' +const MAP_ID = 'map-1' + +const TILE: Tile = { + column: 0, + row: 0, + tileZoomLevel: { + scaleFactor: 1, + width: 256, + height: 512, + originalWidth: 256, + originalHeight: 512, + columns: 8, + rows: 8 + }, + imageSize: [2048, 4096] +} + +const IMAGE_DATA: ImageData = { + colorSpace: 'srgb', + data: new Uint8ClampedArray(4), + width: 1, + height: 1 +} + +/** + * A tile talking to its worker across a real comlink boundary, over a + * MessageChannel rather than a Worker. Real comlink is what makes the channel + * count below meaningful: a `proxy()` argument is only turned into a + * MessageChannel by comlink's own transfer handler, so a hand written double + * would measure nothing. + * + * The worker's `getImageData` hangs until `finish` is called, so a test can act + * while the tile is still fetching. + */ +function createTileOnRealComlink() { + const calls: unknown[][] = [] + const aborted: string[] = [] + const released: number[] = [] + let finish: () => void = () => undefined + let fail: (error: Error) => void = () => undefined + + const { port1, port2 } = new MessageChannel() + expose( + { + async getImageData(...args: unknown[]) { + calls.push(args) + await new Promise((resolve, reject) => { + finish = resolve + fail = reject + }) + return IMAGE_DATA + }, + abort(tileUrl: string) { + aborted.push(tileUrl) + finish() + } + }, + port1 + ) + const worker = wrap(port2) + + // `WorkerPool` keeps private fields, so a stand-in cannot satisfy it + // structurally. Everything else here is the real type. + const workerPool = { + acquire: () => ({ worker, index: 0 }), + release: (index: number) => released.push(index) + } as unknown as WorkerPool + + const spritesWorker = wrap( + new MessageChannel().port1 + ) + + // No fetchFn: a function cannot be structured cloned, so passing one across + // this boundary throws DataCloneError. + const tile = new CacheableWorkerImageDataTile( + new FetchableTile(TILE, MAP_ID, TILE_URL), + workerPool, + spritesWorker + ) + + return { + tile, + calls, + aborted, + released, + finish: () => finish(), + fail: (error: Error) => fail(error), + close: () => { + port1.close() + port2.close() + } + } +} + +/** Counts what comlink opens while `run` executes. */ +async function countMessageChannels(run: () => Promise) { + const RealMessageChannel = globalThis.MessageChannel + let opened = 0 + // stubGlobal so afterEach's unstubAllGlobals is the single restore mechanism. + vi.stubGlobal( + 'MessageChannel', + class extends RealMessageChannel { + constructor() { + super() + opened++ + } + } + ) + + await run() + + return opened +} + +/** Waits for a comlink round trip and the handlers that follow it. */ +const settle = () => new Promise((resolve) => setTimeout(resolve, 20)) + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('CacheableWorkerImageDataTile', () => { + test('fetching a tile opens no MessageChannel', async () => { + const { tile, finish, close } = createTileOnRealComlink() + + // comlink turns each `proxy()` argument into a MessageChannel whose port + // stays exposed on this side until the worker's copy is garbage collected. + // One per tile is what a panning user pays, so the only safe number is zero. + const opened = await countMessageChannels(async () => { + await tile.fetch() + await settle() + finish() + await settle() + }) + + expect(opened).toBe(0) + close() + }) + + test('sends the url, fetch function and tile size, in that order', async () => { + const { tile, calls, finish, close } = createTileOnRealComlink() + + await tile.fetch() + await settle() + finish() + await settle() + + expect(calls[0]).toEqual([TILE_URL, undefined, 256, 512]) + close() + }) + + test('aborting tells the worker to stop that fetch', async () => { + const { tile, aborted, close } = createTileOnRealComlink() + + void tile.fetch() + await settle() + tile.abort() + await settle() + + // The base class only trips a flag on this side. Cancelling for real means + // the worker hearing about it. + expect(aborted).toEqual([TILE_URL]) + close() + }) + + test('a failing fetch is reported, so the in-flight count comes back down', async () => { + const { tile, fail, close } = createTileOnRealComlink() + const errors: Event[] = [] + tile.addEventListener(WarpedMapEventType.TILEFETCHERROR, (event) => + errors.push(event) + ) + + void tile.fetch() + await settle() + fail(new Error('network down')) + await settle() + + expect(errors).toHaveLength(1) + close() + }) + + test('aborting after the fetch finished says nothing to the worker', async () => { + const { tile, aborted, finish, close } = createTileOnRealComlink() + + void tile.fetch() + await settle() + finish() + await settle() + tile.abort() + await settle() + + expect(aborted).toEqual([]) + close() + }) + + test('aborting twice sends one abort', async () => { + const { tile, aborted, close } = createTileOnRealComlink() + + void tile.fetch() + await settle() + tile.abort() + tile.abort() + await settle() + + expect(aborted).toEqual([TILE_URL]) + close() + }) + + test('a response arriving after an abort is discarded', async () => { + const { tile, finish, close } = createTileOnRealComlink() + const fetched: Event[] = [] + tile.addEventListener(WarpedMapEventType.TILEFETCHED, (event) => + fetched.push(event) + ) + + void tile.fetch() + await settle() + tile.abort() + finish() + await settle() + + expect(fetched).toEqual([]) + expect(tile.isCachedTile()).toBe(false) + close() + }) + + test('aborting opens no MessageChannel either', async () => { + const { tile, close } = createTileOnRealComlink() + + const opened = await countMessageChannels(async () => { + void tile.fetch() + await settle() + tile.abort() + await settle() + }) + + expect(opened).toBe(0) + close() + }) + + test('returns the worker to the pool', async () => { + const { tile, released, finish, close } = createTileOnRealComlink() + + await tile.fetch() + await settle() + finish() + await settle() + + expect(released).toEqual([0]) + close() + }) +}) + +/** + * `expose` attaches to globalThis when the worker module is imported, and in + * Node there is nothing to attach to, so the endpoint is stubbed before loading. + */ +async function importWorker() { + vi.stubGlobal('addEventListener', () => undefined) + vi.stubGlobal('removeEventListener', () => undefined) + // Fresh module per test: its abortControllers map is what these tests probe. + vi.resetModules() + + const { fetchAndGetImageDataWorker, abortControllers } = + await import('../src/workers/fetch-and-get-image-data.js') + + return { worker: fetchAndGetImageDataWorker, abortControllers } +} + +function stubImageDataBrowserApis() { + const decoded = { closed: 0, drawn: 0, size: [0, 0] as [number, number] } + + vi.stubGlobal('createImageBitmap', async () => ({ + close: () => { + decoded.closed++ + } + })) + vi.stubGlobal( + 'OffscreenCanvas', + class { + constructor(width: number, height: number) { + decoded.size = [width, height] + } + getContext() { + return { + drawImage: () => { + decoded.drawn++ + }, + getImageData: () => IMAGE_DATA + } + } + } + ) + + return decoded +} + +/** A fetch that never returns on its own, only when its signal is aborted. */ +const hangUntilAborted = (_input: unknown, init?: RequestInit) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => + reject(new DOMException('The operation was aborted', 'AbortError')) + ) + }) + +describe('fetch-and-get-image-data worker', () => { + test('fetches the tile with the supplied fetch function', async () => { + const { worker } = await importWorker() + const seen: unknown[] = [] + + const decoded = stubImageDataBrowserApis() + + await worker.getImageData( + TILE_URL, + async (input) => { + seen.push(input) + return new Response(new Blob()) + }, + 2, + 3 + ) + + expect(seen).toEqual([TILE_URL]) + expect(decoded.size).toEqual([2, 3]) + }) + + test('abort stops a fetch that is still running', async () => { + const { worker } = await importWorker() + stubImageDataBrowserApis() + + const fetching = worker.getImageData(TILE_URL, hangUntilAborted, 1, 1) + worker.abort(TILE_URL) + + await expect(fetching).rejects.toThrow(/aborted/i) + }) + + test('a second fetch for the same url stays abortable', async () => { + const { worker } = await importWorker() + stubImageDataBrowserApis() + + let releaseFirst: () => void = () => undefined + const firstFetch = () => + new Promise((resolve) => { + releaseFirst = () => resolve(new Response(new Blob())) + }) + + const first = worker.getImageData(TILE_URL, firstFetch, 1, 1) + const second = worker.getImageData(TILE_URL, hangUntilAborted, 1, 1) + + releaseFirst() + await first + + worker.abort(TILE_URL) + + await expect(second).rejects.toThrow(/abort/i) + }) + + test('the registry empties once a fetch settles', async () => { + const { worker, abortControllers } = await importWorker() + stubImageDataBrowserApis() + + await worker.getImageData( + TILE_URL, + async () => new Response(new Blob()), + 2, + 3 + ) + expect(abortControllers.size).toBe(0) + + // and after a failure, not only after a success + await expect( + worker.getImageData( + TILE_URL, + async () => { + throw new Error('network down') + }, + 2, + 3 + ) + ).rejects.toThrow() + expect(abortControllers.size).toBe(0) + }) + + test('aborting while the body is read stops before decoding', async () => { + const { worker } = await importWorker() + const decoded = stubImageDataBrowserApis() + let releaseBody: () => void = () => undefined + const response = { + ok: true, + blob: () => + new Promise((resolve) => { + releaseBody = () => resolve(new Blob()) + }) + } as unknown as Response + + const fetching = worker.getImageData(TILE_URL, async () => response, 1, 1) + await settle() + worker.abort(TILE_URL) + releaseBody() + + await expect(fetching).rejects.toThrow(/abort/i) + expect(decoded.closed).toBe(0) + expect(decoded.drawn).toBe(0) + }) + + test('aborting while decoding stops before the copy and frees the bitmap', async () => { + const { worker } = await importWorker() + const decoded = stubImageDataBrowserApis() + let releaseDecode: () => void = () => undefined + vi.stubGlobal( + 'createImageBitmap', + () => + new Promise((resolve) => { + releaseDecode = () => + resolve({ + close: () => { + decoded.closed++ + } + }) + }) + ) + + const fetching = worker.getImageData( + TILE_URL, + async () => new Response(new Blob()), + 1, + 1 + ) + await settle() + worker.abort(TILE_URL) + releaseDecode() + + await expect(fetching).rejects.toThrow(/abort/i) + expect(decoded.drawn).toBe(0) + expect(decoded.closed).toBe(1) + }) + + test('propagates a failing fetch', async () => { + const { worker } = await importWorker() + stubImageDataBrowserApis() + + await expect( + worker.getImageData( + TILE_URL, + async () => { + throw new Error('network down') + }, + 1, + 1 + ) + ).rejects.toThrow() + }) +}) diff --git a/packages/render/test/tile-cache.test.ts b/packages/render/test/tile-cache.test.ts new file mode 100644 index 000000000..8f601190b --- /dev/null +++ b/packages/render/test/tile-cache.test.ts @@ -0,0 +1,153 @@ +import { describe, expect, test } from 'vitest' + +import { TileCache } from '../src/tilecache/TileCache.js' +import { CacheableTile } from '../src/tilecache/CacheableTile.js' +import { FetchableTile } from '../src/tilecache/FetchableTile.js' +import { WarpedMapEvent, WarpedMapEventType } from '../src/shared/events.js' + +import type { Tile } from '@allmaps/types' + +const TILE_URL = 'https://example.com/tile.jpg' +const MAP_ID = 'map-1' + +const TILE: Tile = { + column: 0, + row: 0, + tileZoomLevel: { + scaleFactor: 1, + width: 256, + height: 256, + originalWidth: 256, + originalHeight: 256, + columns: 4, + rows: 4 + }, + imageSize: [1024, 1024] +} + +/** A tile that fetches nothing, so a test can decide how it ends. */ +class ControllableTile extends CacheableTile { + async fetch() { + return this.data + } + + succeed() { + this.data = 'pixels' + this.dispatchEvent( + new WarpedMapEvent(WarpedMapEventType.TILEFETCHED, { + tileUrl: this.fetchableTile.tileUrl + }) + ) + } + + fail() { + this.dispatchTileFetchError(new Error('network down')) + } + + // Sprites take no part in the accounting under test. + async applySprites() {} + spritesDataToCachedTiles() { + return [] + } +} + +function createCache() { + const tiles = new Map() + + const cache = new TileCache((fetchableTile) => { + const tile = new ControllableTile(fetchableTile) + tiles.set(fetchableTile.tileUrl, tile) + return tile + }) + + const allLoaded: Event[] = [] + const loading: Event[] = [] + cache.addEventListener(WarpedMapEventType.ALLREQUESTEDTILESLOADED, (event) => + allLoaded.push(event) + ) + cache.addEventListener(WarpedMapEventType.REQUESTEDTILESLOADING, (event) => + loading.push(event) + ) + + const request = (...tileUrls: string[]) => + cache.requestFetchableTiles( + (tileUrls.length ? tileUrls : [TILE_URL]).map( + (tileUrl) => new FetchableTile(TILE, MAP_ID, tileUrl) + ) + ) + + return { + cache, + request, + allLoaded, + loading, + getTile: (tileUrl = TILE_URL) => tiles.get(tileUrl) + } +} + +describe('TileCache', () => { + test('a failed tile that is then pruned leaves the count at zero', () => { + const { cache, request, getTile } = createCache() + + request() + getTile()?.fail() + // Pruning with no info removes it: it is neither cached nor fetching now. + cache.prune(new Map()) + + // Counting it out twice leaves -1, and a negative count never reads as + // finished, so the next tile to arrive would announce "all loaded". + expect(cache.finished).toBe(true) + }) + + test('the count returns to zero when a tile succeeds', () => { + const { cache, request, allLoaded, getTile } = createCache() + + request() + expect(cache.finished).toBe(false) + + getTile()?.succeed() + + expect(cache.finished).toBe(true) + expect(allLoaded).toHaveLength(1) + }) + + test('announces loading once for a batch, and all loaded when the last lands', () => { + const { request, allLoaded, loading, getTile } = createCache() + const second = 'https://example.com/tile-2.jpg' + + request(TILE_URL, second) + + expect(loading).toHaveLength(1) + expect(allLoaded).toHaveLength(0) + + getTile(TILE_URL)?.succeed() + expect(allLoaded).toHaveLength(0) + + getTile(second)?.succeed() + expect(allLoaded).toHaveLength(1) + expect(loading).toHaveLength(1) + }) + + test('clear announces nothing', () => { + const { cache, request, allLoaded, loading } = createCache() + + request() + const before = { allLoaded: allLoaded.length, loading: loading.length } + cache.clear() + + expect(allLoaded).toHaveLength(before.allLoaded) + expect(loading).toHaveLength(before.loading) + expect(cache.finished).toBe(true) + }) + + test('a tile pruned while still fetching is counted out and aborted', () => { + const { cache, request } = createCache() + + request() + expect(cache.finished).toBe(false) + + cache.prune(new Map()) + + expect(cache.finished).toBe(true) + }) +}) From 7e36dc4ab589cc79b062e7dcedcfa15671866f3b Mon Sep 17 00:00:00 2001 From: Danny Hoek Date: Sat, 15 Aug 2026 00:06:53 +0200 Subject: [PATCH 2/3] address review on tile cancellation Only treat an AbortError as expected when this tile is the one that aborted. An abort from a caller supplied fetchFn would otherwise be swallowed, leaving the tile in the cache's in-flight set forever. Reject anyone awaiting allRequestedTilesLoaded when the cache is cleared, rather than hanging. Resolving would report tiles as ready that were thrown away, and the canvas renderer draws as soon as that promise settles. Drop the try/catch around the worker call. A comlink call reports failure as a rejection and never throws, so that branch could not run. --- .../tilecache/CacheableWorkerImageDataTile.ts | 75 +++++++++---------- packages/render/src/tilecache/TileCache.ts | 46 ++++++++---- .../test/fetch-and-get-image-data.test.ts | 44 ++++++++++- packages/render/test/tile-cache.test.ts | 69 +++++++++++++++-- 4 files changed, 170 insertions(+), 64 deletions(-) diff --git a/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts b/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts index 333265f18..a3d3d0c6e 100644 --- a/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts +++ b/packages/render/src/tilecache/CacheableWorkerImageDataTile.ts @@ -39,51 +39,46 @@ export class CacheableWorkerImageDataTile extends CacheableTile { async fetch() { const { worker, index } = this.#workerPool.acquire() this.#fetchingWorker = worker - try { - worker - .getImageData( - this.fetchableTile.tileUrl, - this.fetchFn, - this.fetchableTile.tile.tileZoomLevel.width, - this.fetchableTile.tile.tileZoomLevel.height + + worker + .getImageData( + this.fetchableTile.tileUrl, + this.fetchFn, + this.fetchableTile.tile.tileZoomLevel.width, + this.fetchableTile.tile.tileZoomLevel.height + ) + .then((response) => { + if (this.abortController.signal.aborted) { + return + } + + this.data = response + this.dispatchEvent( + new WarpedMapEvent(WarpedMapEventType.TILEFETCHED, { + tileUrl: this.fetchableTile.tileUrl + }) ) - .then((response) => { - if (this.abortController.signal.aborted) { - return - } - - this.data = response - this.dispatchEvent( - new WarpedMapEvent(WarpedMapEventType.TILEFETCHED, { - tileUrl: this.fetchableTile.tileUrl - }) - ) - }) - .catch((err) => { - if (!(err instanceof Error && err.name === 'AbortError')) { - // TODO: comlink keeps only message/name/stack, so a - // ResourceFetchError arrives stripped and reports as 'unknown'. - this.dispatchTileFetchError(err) - } - }) - .finally(() => { - this.#fetchingWorker = null - this.#workerPool.release(index) - }) - } catch (err) { - this.#fetchingWorker = null - this.#workerPool.release(index) // release even if setup itself throws synchronously - if (err instanceof Error && err.name === 'AbortError') { - // fetchImage was aborted because viewport was moved and tile - // is no longer needed. This error can be ignored, nothing to do. - } else { - this.dispatchTileFetchError(err) - } - } + }) + .catch((err) => { + if (!this.#isOwnAbortError(err)) { + // TODO: comlink keeps only message/name/stack, so a + // ResourceFetchError arrives stripped and reports as 'unknown'. + this.dispatchTileFetchError(err) + } + }) + .finally(() => { + this.#fetchingWorker = null + this.#workerPool.release(index) + }) return this.data } + /** Our own abort is expected. One we did not ask for is a failure. */ + #isOwnAbortError(err: unknown) { + return this.abortController.signal.aborted && this.isAbortError(err) + } + /** The worker cannot see our AbortSignal, so it has to be told separately. */ override abort() { if (this.abortController.signal.aborted) { diff --git a/packages/render/src/tilecache/TileCache.ts b/packages/render/src/tilecache/TileCache.ts index 480c398a8..94da1b8dd 100644 --- a/packages/render/src/tilecache/TileCache.ts +++ b/packages/render/src/tilecache/TileCache.ts @@ -48,6 +48,11 @@ export class TileCache extends EventTarget { */ #tilesFetching: Set = new Set() + #waitingForTiles: Set<{ + resolve: () => void + reject: (error: Error) => void + }> = new Set() + #boundTileFetched = this.tileFetched.bind(this) #boundTileFetchError = this.tileFetchError.bind(this) #boundTilesFromSpriteTile = this.tilesFromSpriteTile.bind(this) @@ -221,22 +226,12 @@ export class TileCache extends EventTarget { * or in a while, when the last one finishes and ALLREQUESTEDTILESLOADED is fired. */ async allRequestedTilesLoaded(): Promise { - return new Promise((resolve) => { - if (this.finished) { - resolve() - } else { - const listener = () => { - this.removeEventListener( - WarpedMapEventType.ALLREQUESTEDTILESLOADED, - listener - ) - resolve() - } - this.addEventListener( - WarpedMapEventType.ALLREQUESTEDTILESLOADED, - listener - ) - } + if (this.finished) { + return + } + + return new Promise((resolve, reject) => { + this.#waitingForTiles.add({ resolve, reject }) }) } @@ -275,6 +270,10 @@ export class TileCache extends EventTarget { this.mapIdsByTileUrl = new Map() this.tileUrlsByMapId = new Map() this.#tilesFetching = new Set() + + this.#stopWaitingForTiles( + new Error('Tile cache was cleared while waiting for tiles') + ) } destroy() { @@ -610,11 +609,26 @@ export class TileCache extends EventTarget { this.dispatchEvent( new WarpedMapEvent(WarpedMapEventType.ALLREQUESTEDTILESLOADED) ) + this.#stopWaitingForTiles() } return true } + /** With an error, so a cleared cache never reads as loaded. */ + #stopWaitingForTiles(error?: Error) { + const waiting = this.#waitingForTiles + this.#waitingForTiles = new Set() + + for (const { resolve, reject } of waiting) { + if (error) { + reject(error) + } else { + resolve() + } + } + } + protected addEventListenersToCacheableTile(cacheableTile: CacheableTile) { cacheableTile.addEventListener( WarpedMapEventType.TILEFETCHED, diff --git a/packages/render/test/fetch-and-get-image-data.test.ts b/packages/render/test/fetch-and-get-image-data.test.ts index 010e65965..65bb53002 100644 --- a/packages/render/test/fetch-and-get-image-data.test.ts +++ b/packages/render/test/fetch-and-get-image-data.test.ts @@ -46,7 +46,7 @@ const IMAGE_DATA: ImageData = { * The worker's `getImageData` hangs until `finish` is called, so a test can act * while the tile is still fetching. */ -function createTileOnRealComlink() { +function createTileOnRealComlink({ abortRejects = false } = {}) { const calls: unknown[][] = [] const aborted: string[] = [] const released: number[] = [] @@ -66,7 +66,13 @@ function createTileOnRealComlink() { }, abort(tileUrl: string) { aborted.push(tileUrl) - finish() + + // The real worker rejects the fetch it was told to stop. + if (abortRejects) { + fail(new DOMException('The operation was aborted', 'AbortError')) + } else { + finish() + } } }, port1 @@ -193,6 +199,40 @@ describe('CacheableWorkerImageDataTile', () => { close() }) + test('an abort this tile did not ask for is reported', async () => { + const { tile, fail, close } = createTileOnRealComlink() + const errors: Event[] = [] + tile.addEventListener(WarpedMapEventType.TILEFETCHERROR, (event) => + errors.push(event) + ) + + void tile.fetch() + await settle() + // A fetchFn with its own timeout, say. Staying quiet would leave the tile + // in the cache's in-flight set forever. + fail(new DOMException('The operation was aborted', 'AbortError')) + await settle() + + expect(errors).toHaveLength(1) + close() + }) + + test('our own abort is not reported as a failure', async () => { + const { tile, close } = createTileOnRealComlink({ abortRejects: true }) + const errors: Event[] = [] + tile.addEventListener(WarpedMapEventType.TILEFETCHERROR, (event) => + errors.push(event) + ) + + void tile.fetch() + await settle() + tile.abort() + await settle() + + expect(errors).toEqual([]) + close() + }) + test('aborting after the fetch finished says nothing to the worker', async () => { const { tile, aborted, finish, close } = createTileOnRealComlink() diff --git a/packages/render/test/tile-cache.test.ts b/packages/render/test/tile-cache.test.ts index 8f601190b..634fb41e6 100644 --- a/packages/render/test/tile-cache.test.ts +++ b/packages/render/test/tile-cache.test.ts @@ -62,12 +62,16 @@ function createCache() { const allLoaded: Event[] = [] const loading: Event[] = [] + const errors: Event[] = [] cache.addEventListener(WarpedMapEventType.ALLREQUESTEDTILESLOADED, (event) => allLoaded.push(event) ) cache.addEventListener(WarpedMapEventType.REQUESTEDTILESLOADING, (event) => loading.push(event) ) + cache.addEventListener(WarpedMapEventType.TILEFETCHERROR, (event) => + errors.push(event) + ) const request = (...tileUrls: string[]) => cache.requestFetchableTiles( @@ -81,22 +85,34 @@ function createCache() { request, allLoaded, loading, - getTile: (tileUrl = TILE_URL) => tiles.get(tileUrl) + errors, + getTile: (tileUrl = TILE_URL) => { + const tile = tiles.get(tileUrl) + + // Throw rather than return undefined: a test that asserts nothing + // happened would otherwise pass by doing nothing. + if (!tile) { + throw new Error(`no tile was created for ${tileUrl}`) + } + + return tile + } } } describe('TileCache', () => { test('a failed tile that is then pruned leaves the count at zero', () => { - const { cache, request, getTile } = createCache() + const { cache, request, allLoaded, getTile } = createCache() request() - getTile()?.fail() + getTile().fail() // Pruning with no info removes it: it is neither cached nor fetching now. cache.prune(new Map()) // Counting it out twice leaves -1, and a negative count never reads as // finished, so the next tile to arrive would announce "all loaded". expect(cache.finished).toBe(true) + expect(allLoaded).toHaveLength(1) }) test('the count returns to zero when a tile succeeds', () => { @@ -105,7 +121,7 @@ describe('TileCache', () => { request() expect(cache.finished).toBe(false) - getTile()?.succeed() + getTile().succeed() expect(cache.finished).toBe(true) expect(allLoaded).toHaveLength(1) @@ -120,10 +136,10 @@ describe('TileCache', () => { expect(loading).toHaveLength(1) expect(allLoaded).toHaveLength(0) - getTile(TILE_URL)?.succeed() + getTile(TILE_URL).succeed() expect(allLoaded).toHaveLength(0) - getTile(second)?.succeed() + getTile(second).succeed() expect(allLoaded).toHaveLength(1) expect(loading).toHaveLength(1) }) @@ -140,6 +156,47 @@ describe('TileCache', () => { expect(cache.finished).toBe(true) }) + test('a removed tile that fails later is not reported', () => { + const { cache, request, errors, getTile } = createCache() + + request() + const tile = getTile() + cache.prune(new Map()) + tile.fail() + + // Nobody wants this tile anymore, so its failure is not news. + expect(errors).toEqual([]) + }) + + test('waiting for all tiles resolves when the last one lands', async () => { + const { cache, request, getTile } = createCache() + + request() + let settled = false + const waiting = cache.allRequestedTilesLoaded().then(() => { + settled = true + }) + + expect(settled).toBe(false) + getTile().succeed() + await waiting + + expect(settled).toBe(true) + }) + + test('waiting for all tiles rejects when the cache is cleared', async () => { + const { cache, request } = createCache() + + request() + const waiting = cache.allRequestedTilesLoaded() + + cache.clear() + + // Resolving would tell the caller its tiles are ready to draw, and it + // would render the nothing that is left. + await expect(waiting).rejects.toThrow(/cleared/i) + }) + test('a tile pruned while still fetching is counted out and aborted', () => { const { cache, request } = createCache() From 10387b811d0728e3c919e5c349eb12f5cefff345 Mon Sep 17 00:00:00 2001 From: Danny Hoek Date: Sat, 15 Aug 2026 00:14:49 +0200 Subject: [PATCH 3/3] assert that a pruned tile is actually aborted The test named it but only checked the in-flight count, so removing the abort call from the cache left all tests green. --- packages/render/test/tile-cache.test.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/render/test/tile-cache.test.ts b/packages/render/test/tile-cache.test.ts index 634fb41e6..6f1bd7d67 100644 --- a/packages/render/test/tile-cache.test.ts +++ b/packages/render/test/tile-cache.test.ts @@ -27,10 +27,17 @@ const TILE: Tile = { /** A tile that fetches nothing, so a test can decide how it ends. */ class ControllableTile extends CacheableTile { + aborted = false + async fetch() { return this.data } + override abort() { + this.aborted = true + super.abort() + } + succeed() { this.data = 'pixels' this.dispatchEvent( @@ -198,13 +205,15 @@ describe('TileCache', () => { }) test('a tile pruned while still fetching is counted out and aborted', () => { - const { cache, request } = createCache() + const { cache, request, getTile } = createCache() request() + const tile = getTile() expect(cache.finished).toBe(false) cache.prune(new Map()) expect(cache.finished).toBe(true) + expect(tile.aborted).toBe(true) }) })