From 294ac2effd10e4d8fc8310d273e90c6c7f5c17f3 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 21 May 2020 11:22:30 -0700 Subject: [PATCH 1/7] fix: capture error stacks across async calls --- dev/src/bulk-writer.ts | 10 ++++++++-- dev/src/index.ts | 10 ++++++++-- dev/src/pool.ts | 2 +- dev/src/reference.ts | 7 +++++-- dev/src/util.ts | 13 +++++++++++++ dev/src/write-batch.ts | 10 ++++++++-- 6 files changed, 43 insertions(+), 9 deletions(-) diff --git a/dev/src/bulk-writer.ts b/dev/src/bulk-writer.ts index 7ef437620..0817e76e1 100644 --- a/dev/src/bulk-writer.ts +++ b/dev/src/bulk-writer.ts @@ -21,7 +21,7 @@ import {RateLimiter} from './rate-limiter'; import {DocumentReference} from './reference'; import {Timestamp} from './timestamp'; import {Precondition, SetOptions, UpdateData} from './types'; -import {Deferred} from './util'; +import {Deferred, wrapError} from './util'; import {BatchWriteResult, WriteBatch, WriteResult} from './write-batch'; /*! @@ -188,7 +188,13 @@ class BulkCommitBatch { 'The batch should be marked as READY_TO_SEND before committing' ); this.state = BatchState.SENT; - return this.writeBatch.bulkCommit(); + + // Capture the error stack to preserve stack tracing across async calls. + const stack = Error().stack; + + return this.writeBatch.bulkCommit().catch(err => { + throw wrapError(err, stack!); + }); } /** diff --git a/dev/src/index.ts b/dev/src/index.ts index 9fdefe2fb..871c63839 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -53,7 +53,7 @@ import { Settings, UnaryMethod, } from './types'; -import {Deferred, isPermanentRpcError, requestTag} from './util'; +import {Deferred, isPermanentRpcError, requestTag, wrapError} from './util'; import { validateBoolean, validateFunction, @@ -908,9 +908,15 @@ export class Firestore { documentRefsOrReadOptions ); const tag = requestTag(); + + // Capture the error stack to preserve stack tracing across async calls. + const stack = Error().stack; + return this.initializeIfNeeded(tag).then(() => this.getAll_(documents, fieldMask, tag) - ); + ).catch(err => { + throw wrapError(err, stack!); + }); } /** diff --git a/dev/src/pool.ts b/dev/src/pool.ts index eb55c9f90..51c4034f5 100644 --- a/dev/src/pool.ts +++ b/dev/src/pool.ts @@ -17,7 +17,7 @@ import * as assert from 'assert'; import {logger} from './logger'; -import {Deferred} from './util'; +import {Deferred, wrapError} from './util'; /** * An auto-resizing pool that distributes concurrent operations over multiple diff --git a/dev/src/reference.ts b/dev/src/reference.ts index f05a083be..2d6e05a7d 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -48,7 +48,7 @@ import { UpdateData, WhereFilterOp, } from './types'; -import {autoId, requestTag} from './util'; +import {autoId, requestTag, wrapError} from './util'; import { invalidArgumentMessage, validateEnumValue, @@ -1808,12 +1808,15 @@ export class Query { _get(transactionId?: Uint8Array): Promise> { const docs: Array> = []; + // Capture the error stack to preserve stack tracing across async calls. + const stack = Error().stack; + return new Promise((resolve, reject) => { let readTime: Timestamp; this._stream(transactionId) .on('error', err => { - reject(err); + reject(wrapError(err, stack!)); }) .on('data', result => { readTime = result.readTime; diff --git a/dev/src/util.ts b/dev/src/util.ts index d04253699..249f87ad3 100644 --- a/dev/src/util.ts +++ b/dev/src/util.ts @@ -146,3 +146,16 @@ export function isPermanentRpcError( return false; } } + +/** + * Wraps the provided error in a new error that includes the provided stack. + * + * Used to preserve stack traces across async calls. + * @private + */ +export function wrapError(err: Error, stack: string): Error { + let wrappedError = new Error(err.message); + wrappedError.stack += '\nCaused by: ' + stack; + wrappedError.stack += '\nCaused by: ' + err.stack; + return wrappedError; +} diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index 8c01eae1d..d848a2d9e 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -37,7 +37,7 @@ import { UpdateMap, } from './types'; import {DocumentData} from './types'; -import {isObject, isPlainObject, requestTag} from './util'; +import {isObject, isPlainObject, requestTag, wrapError} from './util'; import { customObjectMessage, invalidArgumentMessage, @@ -532,7 +532,13 @@ export class WriteBatch { * }); */ commit(): Promise { - return this.commit_(); + + // Capture the error stack to preserve stack tracing across async calls. + const stack = Error().stack; + + return this.commit_().catch(err => { + throw wrapError(err, stack!); + }); } /** From 541449ab15333f8ea0975b2368ae8d3a0705deb6 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 21 May 2020 16:04:32 -0700 Subject: [PATCH 2/7] add comments + test --- dev/src/bulk-writer.ts | 4 ++-- dev/src/index.ts | 4 ++-- dev/src/pool.ts | 2 +- dev/src/reference.ts | 4 ++-- dev/src/util.ts | 2 +- dev/src/write-batch.ts | 5 ++--- dev/system-test/firestore.ts | 11 +++++++++++ 7 files changed, 21 insertions(+), 11 deletions(-) diff --git a/dev/src/bulk-writer.ts b/dev/src/bulk-writer.ts index 0817e76e1..02bd6619f 100644 --- a/dev/src/bulk-writer.ts +++ b/dev/src/bulk-writer.ts @@ -190,10 +190,10 @@ class BulkCommitBatch { this.state = BatchState.SENT; // Capture the error stack to preserve stack tracing across async calls. - const stack = Error().stack; + const stack = Error().stack!; return this.writeBatch.bulkCommit().catch(err => { - throw wrapError(err, stack!); + throw wrapError(err, stack); }); } diff --git a/dev/src/index.ts b/dev/src/index.ts index 871c63839..1698a2119 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -910,12 +910,12 @@ export class Firestore { const tag = requestTag(); // Capture the error stack to preserve stack tracing across async calls. - const stack = Error().stack; + const stack = Error().stack!; return this.initializeIfNeeded(tag).then(() => this.getAll_(documents, fieldMask, tag) ).catch(err => { - throw wrapError(err, stack!); + throw wrapError(err, stack); }); } diff --git a/dev/src/pool.ts b/dev/src/pool.ts index 51c4034f5..eb55c9f90 100644 --- a/dev/src/pool.ts +++ b/dev/src/pool.ts @@ -17,7 +17,7 @@ import * as assert from 'assert'; import {logger} from './logger'; -import {Deferred, wrapError} from './util'; +import {Deferred} from './util'; /** * An auto-resizing pool that distributes concurrent operations over multiple diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 2d6e05a7d..690e1f794 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -1809,14 +1809,14 @@ export class Query { const docs: Array> = []; // Capture the error stack to preserve stack tracing across async calls. - const stack = Error().stack; + const stack = Error().stack!; return new Promise((resolve, reject) => { let readTime: Timestamp; this._stream(transactionId) .on('error', err => { - reject(wrapError(err, stack!)); + reject(wrapError(err, stack)); }) .on('data', result => { readTime = result.readTime; diff --git a/dev/src/util.ts b/dev/src/util.ts index 249f87ad3..d09cbe9d8 100644 --- a/dev/src/util.ts +++ b/dev/src/util.ts @@ -154,7 +154,7 @@ export function isPermanentRpcError( * @private */ export function wrapError(err: Error, stack: string): Error { - let wrappedError = new Error(err.message); + const wrappedError = new Error(err.message); wrappedError.stack += '\nCaused by: ' + stack; wrappedError.stack += '\nCaused by: ' + err.stack; return wrappedError; diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index d848a2d9e..408845189 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -532,12 +532,11 @@ export class WriteBatch { * }); */ commit(): Promise { - // Capture the error stack to preserve stack tracing across async calls. - const stack = Error().stack; + const stack = Error().stack!; return this.commit_().catch(err => { - throw wrapError(err, stack!); + throw wrapError(err, stack); }); } diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index 37b2b6b9d..974fbb106 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -2148,6 +2148,17 @@ describe('WriteBatch class', () => { }); }); + it('has a full stack trace if set() errors', () => { + // Use an invalid document name that the backend will reject. + const ref = randomCol.doc('__doc__'); + const batch = firestore.batch(); + batch.set(ref, {foo: 'a'}); + return batch.commit().then(() => Promise.reject('commit() should have failed') + ).catch((err: Error) => { + expect(err.stack).to.contain('WriteBatch.commit'); + }); + }); + it('has update() method', () => { const ref = randomCol.doc('doc'); const batch = firestore.batch(); From 9da29bf7ea8c60ad0c95177e00f19981437b8b7e Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 21 May 2020 17:48:03 -0700 Subject: [PATCH 3/7] lint --- dev/src/index.ts | 10 +++++----- dev/system-test/firestore.ts | 10 ++++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index 1698a2119..79b93b88d 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -912,11 +912,11 @@ export class Firestore { // Capture the error stack to preserve stack tracing across async calls. const stack = Error().stack!; - return this.initializeIfNeeded(tag).then(() => - this.getAll_(documents, fieldMask, tag) - ).catch(err => { - throw wrapError(err, stack); - }); + return this.initializeIfNeeded(tag) + .then(() => this.getAll_(documents, fieldMask, tag)) + .catch(err => { + throw wrapError(err, stack); + }); } /** diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index 974fbb106..5b5822eb7 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -2153,10 +2153,12 @@ describe('WriteBatch class', () => { const ref = randomCol.doc('__doc__'); const batch = firestore.batch(); batch.set(ref, {foo: 'a'}); - return batch.commit().then(() => Promise.reject('commit() should have failed') - ).catch((err: Error) => { - expect(err.stack).to.contain('WriteBatch.commit'); - }); + return batch + .commit() + .then(() => Promise.reject('commit() should have failed')) + .catch((err: Error) => { + expect(err.stack).to.contain('WriteBatch.commit'); + }); }); it('has update() method', () => { From 3a4d42100b460240dad607f906c38bb0addb7689 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 21 May 2020 18:15:31 -0700 Subject: [PATCH 4/7] change to GoogleError to preserve error code --- dev/src/util.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dev/src/util.ts b/dev/src/util.ts index d09cbe9d8..1f0683a21 100644 --- a/dev/src/util.ts +++ b/dev/src/util.ts @@ -153,8 +153,9 @@ export function isPermanentRpcError( * Used to preserve stack traces across async calls. * @private */ -export function wrapError(err: Error, stack: string): Error { - const wrappedError = new Error(err.message); +export function wrapError(err: GoogleError, stack: string): Error { + const wrappedError = new GoogleError(err.message); + wrappedError.code = err.code; wrappedError.stack += '\nCaused by: ' + stack; wrappedError.stack += '\nCaused by: ' + err.stack; return wrappedError; From 5523007251dbaf7be7c9dacbd039b74cd2e1f85d Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Tue, 26 May 2020 10:14:01 -0700 Subject: [PATCH 5/7] modify error stack in place rather than create new error --- dev/src/util.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/dev/src/util.ts b/dev/src/util.ts index 1f0683a21..41f08886a 100644 --- a/dev/src/util.ts +++ b/dev/src/util.ts @@ -153,10 +153,7 @@ export function isPermanentRpcError( * Used to preserve stack traces across async calls. * @private */ -export function wrapError(err: GoogleError, stack: string): Error { - const wrappedError = new GoogleError(err.message); - wrappedError.code = err.code; - wrappedError.stack += '\nCaused by: ' + stack; - wrappedError.stack += '\nCaused by: ' + err.stack; - return wrappedError; +export function wrapError(err: Error, stack: string): Error { + err.stack += '-\nCaused by: ' + stack; + return err; } From d53aefbae43cbec3015aebca4f1ed42ad6268131 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Tue, 26 May 2020 14:54:59 -0700 Subject: [PATCH 6/7] throw strings with TODO --- dev/src/util.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dev/src/util.ts b/dev/src/util.ts index 41f08886a..f0c3c0e80 100644 --- a/dev/src/util.ts +++ b/dev/src/util.ts @@ -153,7 +153,12 @@ export function isPermanentRpcError( * Used to preserve stack traces across async calls. * @private */ -export function wrapError(err: Error, stack: string): Error { - err.stack += '-\nCaused by: ' + stack; +export function wrapError(err: Error|string, stack: string): Error { + // TODO(b/157506412): Remove `string` type and clean up any string errors + // that we are throwing. + if (typeof err === 'string') { + throw err; + } + err.stack += '\nCaused by: ' + stack; return err; } From c471b2dca934a62ecd5854e295b2f782896566c4 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Tue, 26 May 2020 17:27:53 -0700 Subject: [PATCH 7/7] lint --- dev/src/util.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/src/util.ts b/dev/src/util.ts index f0c3c0e80..15d5f8dd9 100644 --- a/dev/src/util.ts +++ b/dev/src/util.ts @@ -153,7 +153,7 @@ export function isPermanentRpcError( * Used to preserve stack traces across async calls. * @private */ -export function wrapError(err: Error|string, stack: string): Error { +export function wrapError(err: Error | string, stack: string): Error { // TODO(b/157506412): Remove `string` type and clean up any string errors // that we are throwing. if (typeof err === 'string') {