Skip to content

Commit

Permalink
fix: capture error stacks across async calls (#1088)
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Chen committed May 27, 2020
1 parent ad6b923 commit 7acdd7e
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 10 deletions.
10 changes: 8 additions & 2 deletions dev/src/bulk-writer.ts
Expand Up @@ -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';

/*!
Expand Down Expand Up @@ -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);
});
}

/**
Expand Down
14 changes: 10 additions & 4 deletions dev/src/index.ts
Expand Up @@ -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,
Expand Down Expand Up @@ -908,9 +908,15 @@ export class Firestore {
documentRefsOrReadOptions
);
const tag = requestTag();
return this.initializeIfNeeded(tag).then(() =>
this.getAll_(documents, fieldMask, tag)
);

// 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);
});
}

/**
Expand Down
7 changes: 5 additions & 2 deletions dev/src/reference.ts
Expand Up @@ -48,7 +48,7 @@ import {
UpdateData,
WhereFilterOp,
} from './types';
import {autoId, requestTag} from './util';
import {autoId, requestTag, wrapError} from './util';
import {
invalidArgumentMessage,
validateEnumValue,
Expand Down Expand Up @@ -1808,12 +1808,15 @@ export class Query<T = DocumentData> {
_get(transactionId?: Uint8Array): Promise<QuerySnapshot<T>> {
const docs: Array<QueryDocumentSnapshot<T>> = [];

// 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;
Expand Down
16 changes: 16 additions & 0 deletions dev/src/util.ts
Expand Up @@ -146,3 +146,19 @@ 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 | 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;
}
9 changes: 7 additions & 2 deletions dev/src/write-batch.ts
Expand Up @@ -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,
Expand Down Expand Up @@ -532,7 +532,12 @@ export class WriteBatch {
* });
*/
commit(): Promise<WriteResult[]> {
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);
});
}

/**
Expand Down
13 changes: 13 additions & 0 deletions dev/system-test/firestore.ts
Expand Up @@ -2148,6 +2148,19 @@ 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();
Expand Down

0 comments on commit 7acdd7e

Please sign in to comment.