From 8f2d62c64074c5db702cd6313113da72c481a339 Mon Sep 17 00:00:00 2001 From: Marcel Pursche Date: Mon, 27 Jan 2020 15:47:47 +0100 Subject: [PATCH] HARP-8560 Fix for missing tile data bug. * Don't remove tile loaders from cache that are still in use. * Refactoring in TransferManager. * Added more unit tests. Signed-off-by: Marcel Pursche --- .../lib/TileDataSource.ts | 7 +- @here/harp-mapview-decoder/lib/TileLoader.ts | 4 +- .../test/TileDataSourceTest.ts | 51 ++++++++++++- @here/harp-transfer-manager/package.json | 2 + .../src/DeferredPromise.ts | 40 ++++++++++ .../src/TransferManager.ts | 76 +++++-------------- .../test/DeferredPromiseTest.ts | 68 +++++++++++++++++ 7 files changed, 187 insertions(+), 61 deletions(-) create mode 100644 @here/harp-transfer-manager/src/DeferredPromise.ts create mode 100644 @here/harp-transfer-manager/test/DeferredPromiseTest.ts diff --git a/@here/harp-mapview-decoder/lib/TileDataSource.ts b/@here/harp-mapview-decoder/lib/TileDataSource.ts index eb8a7b6657..30aad5be9a 100644 --- a/@here/harp-mapview-decoder/lib/TileDataSource.ts +++ b/@here/harp-mapview-decoder/lib/TileDataSource.ts @@ -175,6 +175,9 @@ export class TileDataSource extends DataSource { // Cancel any pending downloads as early as possible. tileLoader.cancel(); }; + this.m_tileLoaderCache.canEvict = (_, tileLoader) => { + return tileLoader.frameNumLastUsed < this.mapView.frameNumber; + }; } /** @override */ @@ -261,6 +264,7 @@ export class TileDataSource extends DataSource { const mortonCode = tileKey.mortonCode(); const tileLoader = this.m_tileLoaderCache.get(mortonCode); if (tileLoader !== undefined) { + tileLoader.frameNumLastUsed = this.mapView.frameNumber; tile.tileLoader = tileLoader; } else { const newTileLoader = new TileLoader( @@ -268,7 +272,8 @@ export class TileDataSource extends DataSource { tileKey, this.m_options.dataProvider, this.decoder, - 0 + 0, + this.mapView.frameNumber ); tile.tileLoader = newTileLoader; tile.copyrightInfo = this.m_options.copyrightInfo; diff --git a/@here/harp-mapview-decoder/lib/TileLoader.ts b/@here/harp-mapview-decoder/lib/TileLoader.ts index c39be77576..82faf5e042 100644 --- a/@here/harp-mapview-decoder/lib/TileLoader.ts +++ b/@here/harp-mapview-decoder/lib/TileLoader.ts @@ -87,13 +87,15 @@ export class TileLoader { * @param dataProvider The [[DataProvider]] that retrieves the binary tile data. * @param tileDecoder The [[ITileDecoder]] that decodes the binary tile to a [[DecodeTile]]. * @param priority The priority given to the loading job. Highest number will be served first. + * @param frameNumLastUsed The frame number the tile loader was last used. */ constructor( protected dataSource: DataSource, protected tileKey: TileKey, protected dataProvider: DataProvider, protected tileDecoder: ITileDecoder, - public priority: number + public priority: number, + public frameNumLastUsed: number = -1 ) {} /** diff --git a/@here/harp-mapview-decoder/test/TileDataSourceTest.ts b/@here/harp-mapview-decoder/test/TileDataSourceTest.ts index 86115be0c7..a51bfc00d6 100644 --- a/@here/harp-mapview-decoder/test/TileDataSourceTest.ts +++ b/@here/harp-mapview-decoder/test/TileDataSourceTest.ts @@ -79,7 +79,8 @@ function createMockMapView() { projection: webMercatorProjection, // tslint:disable-next-line:no-empty getDataSourceByName() {}, - statistics: new Statistics() + statistics: new Statistics(), + frameNumber: 0 } as any) as MapView; } @@ -386,4 +387,52 @@ describe("TileDataSource", function() { // assert assert.equal(tile.hasGeometry, true); }); + + it("Currently used tile loaders aren't canceled", async function() { + const mockedDataSource = getDataSource(); + const { testedDataSource } = mockedDataSource; + const numTiles = (testedDataSource as any).getCacheCount() + 16; + + // Request more tile loaders that fit into tile loader cache + const tiles: Tile[] = []; + for (let i = 0; i < numTiles; ++i) { + const tile = testedDataSource.getTile(TileKey.fromMortonCode(i)); + tiles.push(tile!); + } + + // Check that no tile loader was canceled + for (const tile of tiles) { + assert.notEqual(tile.tileLoader!.state, TileLoaderState.Canceled); + + await tile.tileLoader!.waitSettled(); + assert(tile.tileLoader!.isFinished); + } + }); + + it("Cancels tile loaders evicted from cache", async function() { + const mockedDataSource = getDataSource(); + const { testedDataSource } = mockedDataSource; + const mapView = testedDataSource.mapView; + const numTiles = (testedDataSource as any).getCacheCount() + 16; + + // Request that should be canceled + const tileToCancel = testedDataSource.getTile(TileKey.fromMortonCode(0))!; + + // Request more tiles for next frame + (mapView as any).frameNumber = 1; + const tiles: Tile[] = []; + for (let i = 1; i < numTiles; ++i) { + const tile = testedDataSource.getTile(TileKey.fromMortonCode(i)); + tiles.push(tile!); + } + + // Check that only tile loader used in previous frame was canceled + assert.equal(tileToCancel.tileLoader!.state, TileLoaderState.Canceled); + for (const tile of tiles) { + assert.notEqual(tile.tileLoader!.state, TileLoaderState.Canceled); + + await tile.tileLoader!.waitSettled(); + assert(tile.tileLoader!.isFinished); + } + }); }); diff --git a/@here/harp-transfer-manager/package.json b/@here/harp-transfer-manager/package.json index 95fde8c2dc..ca5ad6a6ec 100644 --- a/@here/harp-transfer-manager/package.json +++ b/@here/harp-transfer-manager/package.json @@ -28,9 +28,11 @@ }, "devDependencies": { "@types/chai": "^4.1.2", + "@types/chai-as-promised": "^7.1.2", "@types/mocha": "^5.2.7", "@types/sinon": "^7.0.13", "chai": "^4.0.2", + "chai-as-promised": "^7.1.1", "cross-env": "^6.0.3", "glob": "^7.1.1", "mocha": "^6.1.4", diff --git a/@here/harp-transfer-manager/src/DeferredPromise.ts b/@here/harp-transfer-manager/src/DeferredPromise.ts new file mode 100644 index 0000000000..5eba0aabf9 --- /dev/null +++ b/@here/harp-transfer-manager/src/DeferredPromise.ts @@ -0,0 +1,40 @@ +/* + * Copyright (C) 2017-2019 HERE Europe B.V. + * Licensed under Apache 2.0, see full license in LICENSE + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * + * DeferredPromise takes an executor function for executing it later, when [[exec]] is called. + * This class allows wrapping other promises or long running functions for later execution. + * @internal + */ +export class DeferredPromise { + /** + * Internal promise to store the result of the deferred executor function. + */ + readonly promise: Promise; + private resolveFunc?: (result?: T) => void; + private rejectFunc?: (reason?: any) => void; + + /** + * Constructs a new [[DeferredPromise]] + * @param executor Async function that should be executed at a later point in time. + */ + constructor(private readonly executor: () => Promise) { + this.promise = new Promise((resolve, reject) => { + this.resolveFunc = resolve; + this.rejectFunc = reject; + }); + } + + /** + * When `exec` is called the deferred executor function is executed. + */ + exec() { + this.executor() + .then(result => this.resolveFunc!(result)) + .catch(error => this.rejectFunc!(error)); + } +} diff --git a/@here/harp-transfer-manager/src/TransferManager.ts b/@here/harp-transfer-manager/src/TransferManager.ts index 95fb6b2a20..53ad513a57 100644 --- a/@here/harp-transfer-manager/src/TransferManager.ts +++ b/@here/harp-transfer-manager/src/TransferManager.ts @@ -11,53 +11,7 @@ */ import "@here/harp-fetch"; - -/** @internal - * DeferredPromise takes an executor function for executing it later, when [[exec]] is called. - * This class allows wrapping other promises or long running functions for later execution. - */ -class DeferredPromise { - /** - * Internal promise to store the deferred executor function. - */ - readonly promise: Promise; - private doExec = false; - private resolveFunc?: (result?: T) => void; - private rejectFunc?: (reason?: any) => void; - - constructor(private readonly executor: () => Promise) { - this.promise = new Promise((resolve, reject) => { - this.resolveFunc = resolve; - this.rejectFunc = reject; - - if (this.doExec) { - this.execInnerPromise(this.resolveFunc, this.rejectFunc); - } - }); - } - - /** - * When `exec` is called the deferred executor function is executed. - */ - exec() { - if (this.resolveFunc === undefined || this.rejectFunc === undefined) { - // deferred promise not yet initialized - handle it in callback above - this.doExec = true; - return; - } - - this.execInnerPromise(this.resolveFunc, this.rejectFunc); - } - - private execInnerPromise( - resolveFunc: (result?: T) => void, - rejectFunc: (reason?: any) => void - ) { - this.executor() - .then(result => resolveFunc(result)) - .catch(err => rejectFunc(err)); - } -} +import { DeferredPromise } from "./DeferredPromise"; /** * `TransferManager` for downloading URLs. @@ -170,17 +124,23 @@ export class TransferManager { } return this.doDownload(url, init); } - private doDownload(url: string, init?: RequestInit): Promise { - ++this.activeDownloadCount; - return TransferManager.fetchRepeatedly(this.fetchFunction, 0, this.maxRetries, url, init) - .then(response => { - this.onDownloadDone(); - return response; - }) - .catch(err => { - this.onDownloadDone(); - throw err; - }); + private async doDownload(url: string, init?: RequestInit): Promise { + try { + ++this.activeDownloadCount; + const response = await TransferManager.fetchRepeatedly( + this.fetchFunction, + 0, + this.maxRetries, + url, + init + ); + + this.onDownloadDone(); + return response; + } catch (error) { + this.onDownloadDone(); + throw error; + } } private onDownloadDone() { --this.activeDownloadCount; diff --git a/@here/harp-transfer-manager/test/DeferredPromiseTest.ts b/@here/harp-transfer-manager/test/DeferredPromiseTest.ts new file mode 100644 index 0000000000..50ce26fef7 --- /dev/null +++ b/@here/harp-transfer-manager/test/DeferredPromiseTest.ts @@ -0,0 +1,68 @@ +/* + * Copyright (C) 2017-2019 HERE Europe B.V. + * Licensed under Apache 2.0, see full license in LICENSE + * SPDX-License-Identifier: Apache-2.0 + */ + +// tslint:disable:completed-docs +// tslint:disable:only-arrow-functions +// Mocha discourages using arrow functions, see https://mochajs.org/#arrow-functions + +import * as chai from "chai"; +import * as chaiAsPromised from "chai-as-promised"; +chai.use(chaiAsPromised); +const { expect } = chai; + +import { DeferredPromise } from "../src/DeferredPromise"; + +function delay(timeout: number) { + return new Promise(resolve => { + setTimeout(resolve, timeout); + }); +} + +describe("DeferredPromise", function() { + it("does not settle Promise unless exec() was called.", async function() { + let isSettled = false; + const deferred = new DeferredPromise(() => { + isSettled = true; + return Promise.resolve(); + }); + + // Delay execution + await delay(1); + + // tslint:disable-next-line: no-unused-expression + expect(deferred.promise).to.not.be.undefined; + // tslint:disable-next-line: no-unused-expression + expect(isSettled).to.be.false; + }); + + it("settles a Promise when exec() is called.", async function() { + let isSettled = false; + const deferred = new DeferredPromise(() => { + isSettled = true; + return Promise.resolve(); + }); + + deferred.exec(); + await expect(deferred.promise).to.eventually.be.fulfilled; + + // tslint:disable-next-line: no-unused-expression + expect(isSettled).to.be.true; + }); + + it("handles a rejected Promise.", async function() { + let isSettled = false; + const deferred = new DeferredPromise(() => { + isSettled = true; + return Promise.reject("Some error happened."); + }); + + deferred.exec(); + await expect(deferred.promise).to.eventually.be.rejectedWith("Some error happened."); + + // tslint:disable-next-line: no-unused-expression + expect(isSettled).to.be.true; + }); +});