Skip to content

port public shutdown to web sdk. #2045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
12 commits merged into from
Aug 7, 2019
35 changes: 33 additions & 2 deletions packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import * as firestore from '@firebase/firestore-types';

import { FirebaseApp } from '@firebase/app-types';
import { FirebaseService } from '@firebase/app-types/private';
import { FirebaseService, _FirebaseApp } from '@firebase/app-types/private';
import { DatabaseId, DatabaseInfo } from '../core/database_info';
import { ListenOptions } from '../core/event_manager';
import {
Expand Down Expand Up @@ -427,7 +427,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
this.makeDatabaseInfo()
);
const deferred = new Deferred<void>();
this._queue.enqueueAndForget(async () => {
this._queue.enqueueAndForgetEvenAfterShutdown(async () => {
try {
if (
this._firestoreClient !== undefined &&
Expand All @@ -447,6 +447,37 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
return deferred.promise;
}

/**
* Shuts down this Firestore instance.
*
* After shutdown only the `clearPersistence()` method may be used. Any other method
* will throw a `FirestoreError`.
*
* To restart after shutdown, simply create a new instance of FirebaseFirestore with
* `firebase.firestore()`.
*
* Shutdown does not cancel any pending writes and any promises that are awaiting a response
* from the server will not be resolved. If you have persistence enabled, the next time you
* start this instance, it will resume attempting to send these writes to the server.
*
* Note: Under normal circumstances, calling `shutdown()` is not required. This
* method is useful only when you want to force this instance to release all of its resources or
* in combination with `clearPersistence()` to ensure that all local state is destroyed
* between test runs.
*
* @return A promise that is resolved when the instance has been successfully shut down.
*/
// TODO(b/135755126): make this public.
_shutdown(): Promise<void> {
(this.app as _FirebaseApp)._removeServiceInstance('firestore');
return this.INTERNAL.delete();
}

get _isShutdown(): boolean {
this.ensureClientConfigured();
return this._firestoreClient!.clientShutdown;
}

ensureClientConfigured(): FirestoreClient {
if (!this._firestoreClient) {
// Kick off starting the client but don't actually wait for it.
Expand Down
35 changes: 17 additions & 18 deletions packages/firestore/src/core/firestore_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ export class FirestoreClient {
private lruScheduler?: LruScheduler;

private readonly clientId = AutoId.newId();
private _clientShutdown = false;

// PORTING NOTE: SharedClientState is only used for multi-tab web.
private sharedClientState: SharedClientState;
Expand Down Expand Up @@ -310,7 +309,7 @@ export class FirestoreClient {
* this class cannot be called after the client is shutdown.
*/
private verifyNotShutdown(): void {
if (this._clientShutdown) {
if (this.asyncQueue.isShuttingDown) {
throw new FirestoreError(
Code.FAILED_PRECONDITION,
'The client has already been shutdown.'
Expand Down Expand Up @@ -507,22 +506,19 @@ export class FirestoreClient {
}

shutdown(): Promise<void> {
return this.asyncQueue.enqueue(async () => {
if (!this._clientShutdown) {
// PORTING NOTE: LocalStore does not need an explicit shutdown on web.
if (this.lruScheduler) {
this.lruScheduler.stop();
}
await this.remoteStore.shutdown();
await this.sharedClientState.shutdown();
await this.persistence.shutdown();

// `removeChangeListener` must be called after shutting down the
// RemoteStore as it will prevent the RemoteStore from retrieving
// auth tokens.
this.credentials.removeChangeListener();
this._clientShutdown = true;
return this.asyncQueue.enqueueAndInitiateShutdown(async () => {
// PORTING NOTE: LocalStore does not need an explicit shutdown on web.
if (this.lruScheduler) {
this.lruScheduler.stop();
}
await this.remoteStore.shutdown();
await this.sharedClientState.shutdown();
await this.persistence.shutdown();

// `removeChangeListener` must be called after shutting down the
// RemoteStore as it will prevent the RemoteStore from retrieving
// auth tokens.
this.credentials.removeChangeListener();
});
}

Expand Down Expand Up @@ -603,7 +599,10 @@ export class FirestoreClient {
}

get clientShutdown(): boolean {
return this._clientShutdown;
// Technically, the asyncQueue is still running, but only accepting operations
// related to shutdown or supposed to be run after shutdown. It is effectively
// shut down to the eyes of users.
return this.asyncQueue.isShuttingDown;
}

transaction<T>(
Expand Down
59 changes: 58 additions & 1 deletion packages/firestore/src/util/async_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ export class AsyncQueue {
// The last promise in the queue.
private tail: Promise<unknown> = Promise.resolve();

// Is this AsyncQueue being shut down? Once it is set to true, it will not
// be changed again.
private _isShuttingDown: boolean = false;

// Operations scheduled to be queued in the future. Operations are
// automatically removed after they are run or canceled.
private delayedOperations: Array<DelayedOperation<unknown>> = [];
Expand All @@ -199,6 +203,12 @@ export class AsyncQueue {
// assertion sanity-checks.
private operationInProgress = false;

// Is this AsyncQueue being shut down? If true, this instance will not enqueue
// any new operations, Promises from enqueue requests will not resolve.
get isShuttingDown(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this name was decided in previous ports, in which case, feel free to just leave it. But I would find "isShutdown" clearer than "isShuttingDown" since the latter sounds like a very transitive state during the actual shutdown process itself, rather than a permanent state.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is from other ports. The reason is async queue is not actually shutdown..so i picked this name originally, it is leaking the details a bit. But async queue is an internal class used frequently, maybe it's not too bad to indicate how it is implemented on the name.

FirestoreClient has clientShutdown instead, because that is one abstraction up.

return this._isShuttingDown;
}

/**
* Adds a new operation to the queue without waiting for it to complete (i.e.
* we ignore the Promise result).
Expand All @@ -208,12 +218,58 @@ export class AsyncQueue {
this.enqueue(op);
}

/**
* Regardless if the queue has initialized shutdown, adds a new operation to the
* queue without waiting for it to complete (i.e. we ignore the Promise result).
*/
enqueueAndForgetEvenAfterShutdown<T extends unknown>(
op: () => Promise<T>
): void {
this.verifyNotFailed();
// tslint:disable-next-line:no-floating-promises
this.enqueueInternal(op);
}

/**
* Regardless if the queue has initialized shutdown, adds a new operation to the
* queue.
*/
private enqueueEvenAfterShutdown<T extends unknown>(
op: () => Promise<T>
): Promise<T> {
this.verifyNotFailed();
return this.enqueueInternal(op);
}

/**
* Adds a new operation to the queue and initialize the shut down of this queue.
* Returns a promise that will be resolved when the promise returned by the new
* operation is (with its value).
* Once this method is called, the only possible way to request running an operation
* is through `enqueueAndForgetEvenAfterShutdown`.
*/
async enqueueAndInitiateShutdown(op: () => Promise<void>): Promise<void> {
this.verifyNotFailed();
if (!this._isShuttingDown) {
this._isShuttingDown = true;
await this.enqueueEvenAfterShutdown(op);
}
}

/**
* Adds a new operation to the queue. Returns a promise that will be resolved
* when the promise returned by the new operation is (with its value).
*/
enqueue<T extends unknown>(op: () => Promise<T>): Promise<T> {
this.verifyNotFailed();
if (this._isShuttingDown) {
// Return a Promise which never resolves.
return new Promise<T>(resolve => {});
}
return this.enqueueInternal(op);
}

private enqueueInternal<T extends unknown>(op: () => Promise<T>): Promise<T> {
const newTail = this.tail.then(() => {
this.operationInProgress = true;
return op()
Expand Down Expand Up @@ -309,7 +365,8 @@ export class AsyncQueue {
* operations are not run.
*/
drain(): Promise<void> {
return this.enqueue(() => Promise.resolve());
// It should still be possible to drain the queue after it is shutting down.
return this.enqueueEvenAfterShutdown(() => Promise.resolve());
}

/**
Expand Down
54 changes: 53 additions & 1 deletion packages/firestore/test/integration/api/database.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ import {
apiDescribe,
arrayContainsAnyOp,
inOp,
shutdownDb,
withTestCollection,
withTestDb,
withTestDbs,
withTestDoc,
withTestDocAndInitialData
withTestDocAndInitialData,
DEFAULT_SETTINGS
} from '../util/helpers';

// tslint:disable:no-floating-promises
Expand Down Expand Up @@ -1068,4 +1070,54 @@ apiDescribe('Database', (persistence: boolean) => {
await db.enableNetwork();
});
});

it('can start a new instance after shut down', async () => {
return withTestDoc(persistence, async docRef => {
const firestore = docRef.firestore;
await shutdownDb(firestore);

const newFirestore = firebase.firestore!(firestore.app);
expect(newFirestore).to.not.equal(firestore);

// New instance functions.
newFirestore.settings(DEFAULT_SETTINGS);
await newFirestore.doc(docRef.path).set({ foo: 'bar' });
const doc = await newFirestore.doc(docRef.path).get();
expect(doc.data()).to.deep.equal({ foo: 'bar' });
});
});

it('app delete leads to instance shutdown', async () => {
await withTestDoc(persistence, async docRef => {
await docRef.set({ foo: 'bar' });
const app = docRef.firestore.app;
await app.delete();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((docRef.firestore as any)._isShutdown).to.be.true;
});
});

it('new operation after shutdown should throw', async () => {
await withTestDoc(persistence, async docRef => {
const firestore = docRef.firestore;
await shutdownDb(firestore);

expect(() => {
firestore.doc(docRef.path).set({ foo: 'bar' });
}).to.throw();
});
});

it('calling shutdown mutiple times should proceed', async () => {
await withTestDoc(persistence, async docRef => {
const firestore = docRef.firestore;
await shutdownDb(firestore);
await shutdownDb(firestore);

expect(() => {
firestore.doc(docRef.path).set({ foo: 'bar' });
}).to.throw();
});
});
});
5 changes: 5 additions & 0 deletions packages/firestore/test/integration/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,11 @@ function wipeDb(db: firestore.FirebaseFirestore): Promise<void> {
return Promise.resolve(undefined);
}

export function shutdownDb(db: firestore.FirebaseFirestore): Promise<void> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return (db as any)._shutdown();
}

// TODO(in-queries): This exists just so we don't have to do the cast
// repeatedly. Once we expose 'array-contains-any' publicly we can remove it and
// just use 'array-contains-any' in all the tests.
Expand Down
21 changes: 21 additions & 0 deletions packages/firestore/test/unit/util/async_queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,27 @@ describe('AsyncQueue', () => {
await queue.drain();
expect(completedSteps).to.deep.equal([1, 2]);
});

it('Schedules operaions with respect to shut down', async () => {
const queue = new AsyncQueue();
const completedSteps: number[] = [];
const doStep = (n: number): Promise<void> =>
defer(() => {
completedSteps.push(n);
});

queue.enqueueAndForget(() => doStep(1));

// After this call, only operations requested via
// `enqueueAndForgetEvenAfterShutdown` gets executed.
// tslint:disable-next-line:no-floating-promises
queue.enqueueAndInitiateShutdown(() => doStep(2));
queue.enqueueAndForget(() => doStep(3));
queue.enqueueAndForgetEvenAfterShutdown(() => doStep(4));

await queue.drain();
expect(completedSteps).to.deep.equal([1, 2, 4]);
});
});

function defer<T>(op: () => T): Promise<T> {
Expand Down