Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

Commit

Permalink
HARP-11487: Ensure DataProvider's connect and disposed are called once.
Browse files Browse the repository at this point in the history
Both methods are now protected. Clients call instead register/unregister,
and new DataProvider abstract class ensures connect is called on first
registered client and dispose called on last unregistered client.

Breaking API change:
- Classes derived from DataProvider must use "extends" instead of "implements",
 and must call super() on constructor.
- Custom Data sources using DataProvider must call register/unregister instead
of connect/dispose.
  • Loading branch information
atomicsulfate committed Aug 24, 2020
1 parent 6ecdb26 commit 7828816
Show file tree
Hide file tree
Showing 12 changed files with 147 additions and 57 deletions.
2 changes: 1 addition & 1 deletion @here/harp-examples/src/datasource_custom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ import { CUSTOM_DECODER_SERVICE_TYPE } from "../decoder/custom_decoder_defs";

export namespace CustomDatasourceExample {
// snippet:custom_datasource_example_custom_data_provider.ts
class CustomDataProvider implements DataProvider
class CustomDataProvider extends DataProvider
// end:custom_datasource_example_custom_data_provider.ts
{
connect() {
Expand Down
2 changes: 1 addition & 1 deletion @here/harp-examples/src/tile_dependencies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export namespace TileDependenciesExample {
</p>
<pre id="mouse-picked-result"></pre>
`;
class CustomDataProvider implements DataProvider {
class CustomDataProvider extends DataProvider {
enableTileDependencies: boolean = false;
connect() {
// Here you could connect to the service.
Expand Down
40 changes: 33 additions & 7 deletions @here/harp-mapview-decoder/lib/DataProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import "@here/harp-fetch";

import { TileKey } from "@here/harp-geoutils";
import { EventDispatcher } from "three";

/**
* Interface for all `DataProvider` subclasses.
Expand All @@ -16,18 +17,36 @@ import { TileKey } from "@here/harp-geoutils";
* loader which is only responsible for loading the data of a specific tile,
* without any relation to displaying or even decoding the data.
*/
export interface DataProvider {
export abstract class DataProvider extends EventDispatcher {
private readonly m_clients: Set<Object> = new Set();

/**
* Connect to the data source. Returns a promise to wait for successful (or failed) connection.
* Registers a client to the data provider.
*
* @returns A promise which is resolved when the connection has been established.
* @param client - The client to register.
* @returns Promise to wait for successful (or failed) connection to the data source.
*/
register(client: Object): Promise<void> {
const result = this.m_clients.size === 0 ? this.connect() : Promise.resolve();
this.m_clients.add(client);
return result;
}

/**
* Unregisters a client from the data provider.
*
* @param client - The client to unregister.
*/
connect(): Promise<void>;
unregister(client: Object) {
if (this.m_clients.delete(client) && this.m_clients.size === 0) {
this.dispose();
}
}

/**
* Returns `true` if it has been connected successfully.
*/
ready(): boolean;
abstract ready(): boolean;

/**
* Load the data of a {@link @here/map-view@Tile} asynchronously.
Expand All @@ -36,7 +55,7 @@ export interface DataProvider {
* @param abortSignal - Optional AbortSignal to cancel the request.
* @returns A promise delivering the data as an [[ArrayBufferLike]], or any object.
*/
getTile(tileKey: TileKey, abortSignal?: AbortSignal): Promise<ArrayBufferLike | {}>;
abstract getTile(tileKey: TileKey, abortSignal?: AbortSignal): Promise<ArrayBufferLike | {}>;

/**
* An event which fires when this `DataProvider` is invalidated.
Expand All @@ -53,9 +72,16 @@ export interface DataProvider {
*/
onDidInvalidate?(listener: () => void): () => void;

/**
* Connect to the data source. Returns a promise to wait for successful (or failed) connection.
*
* @returns A promise which is resolved when the connection has been established.
*/
protected abstract connect(): Promise<void>;

/**
* Destroys this `DataProvider`. Implementations of `DataProvider` must dispose of
* asynchronous operations and services here.
*/
dispose(): void;
protected abstract dispose(): void;
}
12 changes: 8 additions & 4 deletions @here/harp-mapview-decoder/lib/TestDataProviders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ import { DataProvider } from "../lib/DataProvider";
/**
* Data provider that loads test tile using [[loadTestResource]].
*/
export class TestSingleFileDataProvider implements DataProvider {
export class TestSingleFileDataProvider extends DataProvider {
/**
* TestDataProvider constructor
* @param moduleName - name of the module's directory
* @param basePath - base path to the test resources
*/
constructor(private readonly moduleName: string, private readonly basePath: string) {}
constructor(private readonly moduleName: string, private readonly basePath: string) {
super();
}

ready(): boolean {
return true;
Expand All @@ -46,13 +48,15 @@ export class TestSingleFileDataProvider implements DataProvider {
* The URL is constructed using the following formula:
* `${this.basePath}/${tileKey.mortonCode()}.bin`
*/
export class TestTilesDataProvider implements DataProvider {
export class TestTilesDataProvider extends DataProvider {
/**
* Constructs `TestFilesDataProvider` using the provided base path.
*
* @param basePath - base path to be used to construct the url to the resource.
*/
constructor(private readonly basePath: string) {}
constructor(private readonly basePath: string) {
super();
}

ready(): boolean {
return true;
Expand Down
4 changes: 2 additions & 2 deletions @here/harp-mapview-decoder/lib/TileDataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export class TileDataSource<TileType extends Tile = Tile> extends DataSource {
dispose() {
this.m_unregisterClearTileCache?.();
this.decoder.dispose();
this.dataProvider().dispose();
this.dataProvider().unregister(this);
}

/** @override */
Expand All @@ -181,7 +181,7 @@ export class TileDataSource<TileType extends Tile = Tile> extends DataSource {

/** @override */
async connect() {
await Promise.all([this.m_options.dataProvider.connect(), this.m_decoder.connect()]);
await Promise.all([this.m_options.dataProvider.register(this), this.m_decoder.connect()]);
this.m_isReady = true;

this.m_decoder.configure(undefined, undefined, undefined, {
Expand Down
63 changes: 63 additions & 0 deletions @here/harp-mapview-decoder/test/DataProviderTest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright (C) 2020 HERE Europe B.V.
* Licensed under Apache 2.0, see full license in LICENSE
* SPDX-License-Identifier: Apache-2.0
*/

// Mocha discourages using arrow functions, see https://mochajs.org/#arrow-functions

import { expect } from "chai";
import * as sinon from "sinon";

import { DataProvider } from "../index";

class FakeDataProvider extends DataProvider {
/** @override */ async connect() {
// empty implementation
}

ready(): boolean {
return true;
}

async getTile(): Promise<ArrayBufferLike | {}> {
return await Promise.resolve({});
}

/** @override */ dispose() {
// Nothing to be done here.
}
}

describe("DataProvider", function() {
const clients = [{}, {}];

it("calls connect only on first registered client", function() {
const dataProvider = new FakeDataProvider();
const connectSpy = sinon.spy(dataProvider, "connect");

dataProvider.register(clients[0]);
expect(connectSpy.calledOnce).is.true;

dataProvider.register(clients[0]);
dataProvider.register(clients[1]);
expect(connectSpy.calledOnce).is.true;
});

it("calls dispose only on last unregistered client", function() {
const dataProvider = new FakeDataProvider();
const disposeSpy = sinon.spy(dataProvider, "dispose");

dataProvider.register(clients[0]);
dataProvider.register(clients[1]);

dataProvider.unregister(clients[0]);
dataProvider.unregister(clients[0]);
expect(disposeSpy.called).is.false;

dataProvider.unregister(clients[1]);
expect(disposeSpy.calledOnce).is.true;
dataProvider.unregister(clients[1]);
expect(disposeSpy.calledOnce).is.true;
});
});
61 changes: 30 additions & 31 deletions @here/harp-mapview-decoder/test/TileDataSourceTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,26 +21,22 @@ import * as sinon from "sinon";

import { DataProvider, TileDataSource, TileFactory } from "../index";

function createMockDataProvider() {
const mockTemplate: DataProvider = {
async connect() {
// empty implementation
},
ready(): boolean {
return true;
},
async getTile(): Promise<ArrayBuffer> {
return await Promise.resolve(new ArrayBuffer(5));
},
/** @override */ dispose() {
// Nothing to be done here.
}
};
const mock = sinon.stub(mockTemplate);
mock.getTile.callsFake(() => {
return Promise.resolve(new ArrayBuffer(5));
});
return mock;
class MockDataProvider extends DataProvider {
/** @override */ async connect() {
// empty implementation
}

ready(): boolean {
return true;
}

async getTile(tileKey: TileKey, abortSignal?: AbortSignal): Promise<ArrayBuffer> {
return await Promise.resolve(new ArrayBuffer(5));
}

/** @override */ dispose() {
// Nothing to be done here.
}
}

const fakeGeometry = {};
Expand Down Expand Up @@ -93,7 +89,7 @@ describe("TileDataSource", function() {
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "",
tilingScheme: webMercatorTilingScheme,
dataProvider: createMockDataProvider(),
dataProvider: new MockDataProvider(),
decoder
});

Expand All @@ -111,7 +107,7 @@ describe("TileDataSource", function() {
const testedDataSource = new TileDataSource(new TileFactory(CustomTile), {
styleSetName: "",
tilingScheme: webMercatorTilingScheme,
dataProvider: createMockDataProvider(),
dataProvider: new MockDataProvider(),
decoder: createMockTileDecoder()
});
testedDataSource.attach(createMockMapView());
Expand All @@ -122,11 +118,12 @@ describe("TileDataSource", function() {
});

it("#updateTile: tile disposing cancels load, skips decode and tile update", async function() {
const mockDataProvider = createMockDataProvider();
const mockDataProvider = new MockDataProvider();

const abortController = new AbortController();
let getTileToken = abortController.signal;
mockDataProvider.getTile.callsFake((_tileKey: any, cancellationToken: any) => {
const getTileStub = sinon.stub(mockDataProvider, "getTile");
getTileStub.callsFake((_tileKey: any, cancellationToken: any) => {
assert(cancellationToken !== undefined);
assert(cancellationToken instanceof AbortSignal);
getTileToken = cancellationToken;
Expand Down Expand Up @@ -162,14 +159,15 @@ describe("TileDataSource", function() {

assert.notExists(tile.tileLoader);

assert.equal(mockDataProvider.getTile.callCount, 1);
assert.equal(getTileStub.callCount, 1);
assert.equal(getTileToken.aborted, true);
assert.equal(mockDecoder.decodeTile.callCount, 0);
assert.equal(spyTileSetDecodedTile.set.callCount, 0);
});

it("subsequent, overlapping #updateTile calls load & decode tile once", async function() {
const mockDataProvider = createMockDataProvider();
const mockDataProvider = new MockDataProvider();
const getTileSpy = sinon.spy(mockDataProvider, "getTile");
const mockDecoder = createMockTileDecoder();

mockDecoder.decodeTile.resolves(fakeEmptyGeometry);
Expand All @@ -196,13 +194,13 @@ describe("TileDataSource", function() {
await tile.tileLoader!.waitSettled();

// assert
assert.equal(mockDataProvider.getTile.callCount, 1);
assert.equal(getTileSpy.callCount, 1);
assert.equal(mockDecoder.decodeTile.callCount, 1);
assert(spyTileSetDecodedTile.set.calledWith(fakeEmptyGeometry));
});

function getDataSource() {
const mockDataProvider = createMockDataProvider();
const mockDataProvider = new MockDataProvider();
const mockDecoder = createMockTileDecoder();

mockDecoder.decodeTile.resolves(fakeEmptyGeometry);
Expand All @@ -220,6 +218,7 @@ describe("TileDataSource", function() {

it("subsequent, overlapping #getTile calls don't share TileLoader", async function() {
const mockedDataSource = getDataSource();
const getTileSpy = sinon.spy(mockedDataSource.mockDataProvider, "getTile");
const tile1 = mockedDataSource.testedDataSource.getTile(
TileKey.fromRowColumnLevel(0, 0, 0)
)!;
Expand All @@ -235,7 +234,7 @@ describe("TileDataSource", function() {
// Waiting on the first tileloader doesn't influence the second one.
await tile1.tileLoader!.waitSettled();
await tile2.tileLoader!.waitSettled();
assert.equal(mockedDataSource.mockDataProvider.getTile.callCount, 2);
assert.equal(getTileSpy.callCount, 2);
// Check that two tiles are decoded.
assert.equal(mockedDataSource.mockDecoder.decodeTile.callCount, 2);
assert.equal(spyTileSetDecodedTile1.set.callCount, 1);
Expand All @@ -245,7 +244,7 @@ describe("TileDataSource", function() {
});

it("Empty decoded tiles are ignored", async function() {
const mockDataProvider = createMockDataProvider();
const mockDataProvider = new MockDataProvider();
const mockDecoder = createMockTileDecoder();

fakeEmptyGeometry.geometries = [];
Expand Down Expand Up @@ -298,7 +297,7 @@ describe("TileDataSource", function() {
const testedDataSource = new TileDataSource(new TileFactory(Tile), {
styleSetName: "",
tilingScheme: webMercatorTilingScheme,
dataProvider: createMockDataProvider(),
dataProvider: new MockDataProvider(),
decoder: createMockTileDecoder(),
minZoomLevel: 3,
maxZoomLevel: 17
Expand Down
6 changes: 1 addition & 5 deletions @here/harp-mapview-decoder/test/TileLoaderTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,7 @@ class MockDataSource extends DataSource {
}
}

class MockDataProvider implements DataProvider {
constructor() {
// empty implementation
}

class MockDataProvider extends DataProvider {
async connect() {
// empty implementation
}
Expand Down
6 changes: 4 additions & 2 deletions @here/harp-olp-utils/lib/OlpDataProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,13 @@ export interface OlpDataProviderParams {
/**
* [[DataProvider]] implementation for OLP catalogs.
*/
export class OlpDataProvider implements DataProvider {
export class OlpDataProvider extends DataProvider {
private m_versionLayerClient: VersionedLayerClient | undefined;
private m_catalogVersion: number = -1;

constructor(readonly params: OlpDataProviderParams) {}
constructor(readonly params: OlpDataProviderParams) {
super();
}

/**
* Connect to the data source. Returns a promise to wait for successful (or failed) connection.
Expand Down
Loading

0 comments on commit 7828816

Please sign in to comment.