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: capture error stacks across async calls #1088

Merged
merged 9 commits into from May 27, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
11 changes: 11 additions & 0 deletions dev/src/util.ts
Expand Up @@ -146,3 +146,14 @@ 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 {
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