-
Notifications
You must be signed in to change notification settings - Fork 988
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
Changes from 9 commits
8dd846d
2ab4b34
c9e1495
efffc5e
a07588c
3e6bfca
692dfa3
1438df0
eeec389
45c65a1
59ea6b4
8d957d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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>> = []; | ||
|
|
@@ -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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| return this._isShuttingDown; | ||
| } | ||
|
|
||
| /** | ||
| * Adds a new operation to the queue without waiting for it to complete (i.e. | ||
| * we ignore the Promise result). | ||
|
|
@@ -208,12 +218,57 @@ 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(); | ||
| 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() | ||
|
|
@@ -309,7 +364,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()); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,8 @@ import { | |
| withTestDb, | ||
| withTestDbs, | ||
| withTestDoc, | ||
| withTestDocAndInitialData | ||
| withTestDocAndInitialData, | ||
| DEFAULT_SETTINGS | ||
| } from '../util/helpers'; | ||
|
|
||
| // tslint:disable:no-floating-promises | ||
|
|
@@ -1068,4 +1069,53 @@ 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 (firestore as any)._shutdown(); | ||
|
||
|
|
||
| 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(); | ||
|
|
||
| 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 (firestore as any)._shutdown(); | ||
|
|
||
| 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 (firestore as any)._shutdown(); | ||
| await (firestore as any)._shutdown(); | ||
|
|
||
| expect(() => { | ||
| firestore.doc(docRef.path).set({ foo: 'bar' }); | ||
| }).to.throw(); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leading spaces seems off... If you run
yarn lintfrom packages/firestore, it should complain about this (and it's complaining in the CI build too).