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 3 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
10 changes: 8 additions & 2 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();

// 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
13 changes: 13 additions & 0 deletions dev/src/util.ts
Expand Up @@ -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 {
const wrappedError = new Error(err.message);
wrappedError.stack += '\nCaused by: ' + stack;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we modify the stack in 'err' directly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. done.

wrappedError.stack += '\nCaused by: ' + err.stack;
return wrappedError;
}
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
11 changes: 11 additions & 0 deletions dev/system-test/firestore.ts
Expand Up @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am glad this is causing a lint failure :) Doesn't look pretty like this. The PR is good to go after yarn gts fix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. Also I changed wrapError() to use GoogleError in order to preserve error codes when re-throwing.

).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