feat: allow UnhandledPromiseRejection errors in BulkWriter if no error handler is specified #1572
Conversation
…r handler is specified
product-auto-label
bot
added
the
api: firestore
google-cla
bot
added
the
cla: yes
dev/src/bulk-writer.ts
Outdated
| * @internal | ||
| */ | ||
| private _silencePromiseIfErrorHandlerSet(promise: Promise<unknown>): void { | ||
| if (this._errorHandlerSet) { |
There was a problem hiding this comment.
Can we do this slightly later and make this decision when the operation returns? This would allow the following:
bulkWriter.set();
bulkWriter.set();
bulkWriter.onError(...);
Note that this error handler will always be attached before any result is returned.
There was a problem hiding this comment.
What's the use case for changing the error handler after an operation has been enqueued but before it resolves? To me, it seems like an anti-pattern, or an accidental bug.
There was a problem hiding this comment.
It's guaranteed that this code executes before the request is processed, so I don't think we should forbid this. It is not possible for any network task to resolve before all synchronous code finished executing. I do think it is cleaner to attach the error handler first, but we do not enforce this, so I think we should allow this even if we believe this to be an anti-pattern.
dev/src/bulk-writer.ts
Outdated
| // | ||
| // This is done here in order to chain the caught promise onto `lastOp`, | ||
| // which ensures that flush() resolves after the operation promise. | ||
| const returnedBulkWriterPromise = bulkWriterOp.promise.catch(err => { |
There was a problem hiding this comment.
This should probably use "op" instead of BulkWriter since it only affects a single operation.
I would have also used the term "user" in this variable, but that may be the lack of oxygen at my current elevation (userPromise).
dev/test/bulk-writer.ts
Outdated
| @@ -54,6 +54,7 @@ import { | |||
| verifyInstance, | |||
| } from './util/helpers'; | |||
| import api = proto.google.firestore.v1; | |||
| import {Deferred} from '../src/util'; | |||
There was a problem hiding this comment.
Can you move this up to the other src imports?
| * @private | ||
| * @internal | ||
| */ | ||
| private _errorHandlerSet = false; |
There was a problem hiding this comment.
I don't think you need this. You can just do just do:
const DEFAULT_ERROR_FUNCTION = error => {
const isRetryableDeleteError =
error.operationType === 'delete' &&
(error.code as number) === StatusCode.INTERNAL;
const retryCodes = getRetryCodes('batchWrite');
return (
(retryCodes.includes(error.code) || isRetryableDeleteError) &&
error.failedAttempts < MAX_RETRY_ATTEMPTS
);
};
private _errorFn = DEFAULT_ERROR_FUNCTION;
if (this._errorFn !== DEFAULT_ERROR_FUNCTION) {...}
Up to you.
There was a problem hiding this comment.
I remember wanting to do something similar for the type converters, and we had to use .toString(). I'd prefer keeping it simpler and (imo) more readable with a boolean variable.
🤖 I have created a release \*beep\* \*boop\* --- ## [4.14.0](https://www.github.com/googleapis/nodejs-firestore/compare/v4.13.2...v4.14.0) (2021-07-30) ### Features * add "NON_EMPTY_DEFAULT" FieldBehavior ([#1554](https://www.github.com/googleapis/nodejs-firestore/issues/1554)) ([8d9c503](https://www.github.com/googleapis/nodejs-firestore/commit/8d9c50381eedf6ee8043eed681d03b44262b9820)) * allow UnhandledPromiseRejection errors in BulkWriter if no error handler is specified ([#1572](https://www.github.com/googleapis/nodejs-firestore/issues/1572)) ([e862ac8](https://www.github.com/googleapis/nodejs-firestore/commit/e862ac81cbb99287a226989b184fc2e683defa16)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
BulkWriter should throw an error if an operation promise is rejected without a custom error handler. Otherwise, failures are silently swallowed without any indication to the end-developer. If an error handler is specified with
onWriteError(), BulkWriter assumes that the developer is handling the error and will swallow failed promises so that the developer does not have to add a danglingcatch()statement for every promise.