Skip to content
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

fix: close default BulkWriter upon terminate. #2276

Merged
merged 3 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions dev/src/bulk-writer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -399,13 +399,13 @@ export class BulkWriter {
private _lastOp: Promise<void> = Promise.resolve();

/**
* Whether this BulkWriter instance has started to close. Afterwards, no
* new operations can be enqueued, except for retry operations scheduled by
* the error handler.
* When this BulkWriter instance has started to close, a flush promise is
* saved. Afterwards, no new operations can be enqueued, except for retry
* operations scheduled by the error handler.
* @private
* @internal
*/
private _closing = false;
private _closePromise: Promise<void> | undefined;

/**
* Rate limiter used to throttle requests as per the 500/50/5 rule.
Expand Down Expand Up @@ -921,11 +921,11 @@ export class BulkWriter {
* ```
*/
close(): Promise<void> {
this._verifyNotClosed();
this.firestore._decrementBulkWritersCount();
const flushPromise = this.flush();
this._closing = true;
return flushPromise;
if (!this._closePromise) {
this._closePromise = this.flush();
this.firestore._decrementBulkWritersCount();
}
return this._closePromise;
}

/**
Expand All @@ -934,7 +934,7 @@ export class BulkWriter {
* @internal
*/
_verifyNotClosed(): void {
if (this._closing) {
if (this._closePromise) {
throw new Error('BulkWriter has already been closed.');
}
}
Expand Down
6 changes: 5 additions & 1 deletion dev/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1487,7 +1487,11 @@ export class Firestore implements firestore.Firestore {
*
* @return A Promise that resolves when the client is terminated.
*/
terminate(): Promise<void> {
async terminate(): Promise<void> {
if (this._bulkWriter) {
await this._bulkWriter.close();
this._bulkWriter = undefined;
}
if (this.registeredListenersCount > 0 || this.bulkWritersCount > 0) {
return Promise.reject(
'All onSnapshot() listeners must be unsubscribed, and all BulkWriter ' +
Expand Down
7 changes: 6 additions & 1 deletion dev/system-test/firestore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6967,7 +6967,11 @@ describe('BulkWriter class', () => {
writer = firestore.bulkWriter();
});

afterEach(() => verifyInstance(firestore));
afterEach(async () => {
await writer.close();
await verifyInstance(firestore);
await firestore.terminate();
});

it('has create() method', async () => {
const ref = randomCol.doc('doc1');
Expand Down Expand Up @@ -7131,6 +7135,7 @@ describe('BulkWriter class', () => {
});
await firestore.recursiveDelete(randomCol, bulkWriter);
expect(callbackCount).to.equal(6);
await bulkWriter.close();
});
});

Expand Down
4 changes: 3 additions & 1 deletion dev/test/bulk-writer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,9 @@ describe('BulkWriter', () => {
expect(() => bulkWriter.create(doc, {})).to.throw(expected);
expect(() => bulkWriter.update(doc, {})).to.throw(expected);
expect(() => bulkWriter.flush()).to.throw(expected);
expect(() => bulkWriter.close()).to.throw(expected);

// Calling close() multiple times is allowed.
await bulkWriter.close();
});

it('send writes to the same documents in the different batches', async () => {
Expand Down
Loading