fix: manually retry ABORTED reads in transactions #883

schmidt-sebastian · 2020-01-14T23:36:44Z

This adds manual retry to our transaction logic similar to what we have in the Web SDK: https://github.com/firebase/firebase-js-sdk/blob/c822e78b00dd3420dcc749beb2f09a947aa4a344/packages/firestore/src/core/transaction_runner.ts#L112

We only retry GRPC exceptions, and only failures with code ABORTED. This is because:

we don't want to retry if the user throws an error or returns a rejected Promise
GAPIC retries reads that fail with UNAVAILABLE, INTERNAL and DEADLINE_EXCEEDED under the hood

Fixes #827 (fingers crossed)

codecov · 2020-01-14T23:49:02Z

Codecov Report

Merging #883 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #883      +/-   ##
=========================================
- Coverage    96.6%   96.6%   -0.01%     
=========================================
  Files          25      25              
  Lines       15564   15559       -5     
  Branches     1155    1155              
=========================================
- Hits        15036   15031       -5     
  Misses        519     519              
  Partials        9       9

Impacted Files	Coverage Δ
dev/src/types.ts	`100% <ø> (ø)`	⬆️
dev/src/index.ts	`98.44% <100%> (-0.01%)`	⬇️
dev/src/reference.ts	`98.91% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c66aba...06c482e. Read the comment docs.

thebrianchen

lgtm

thebrianchen · 2020-01-15T00:58:49Z

dev/src/transaction.ts

@@ -552,3 +561,14 @@ function validateReadOptions(
    }
  }
 }
+
+function isRetryableTransactionError(error: Error): boolean {
+  if (error instanceof GoogleError || 'code' in error) {


Haven't seen this elsewhere: Is 'code' in error the standard way to check if the 'code' field exists?

Yes, that is JavaScript functionality (see https://masteringjs.io/tutorials/fundamentals/hasownproperty for a comparison against Object.hasOwnProperty). I am doing it here because I am afraid that some of our dependencies might throw a GoogleError that is not actually a GoogleError by providing just a code and a message field.

thebrianchen · 2020-01-15T01:00:37Z

dev/test/transaction.ts

@@ -106,7 +106,7 @@ function commit(

 function rollback(
  transaction?: Uint8Array | string,
-  err?: Error
+  error?: Error


optional: change naming for commit() as well so they're all in sync (link)

Done. Good catch.

thebrianchen · 2020-01-15T01:02:06Z

dev/test/transaction.ts

+      begin('foo2', 'foo1'),
+      getDocument('foo2'),
+      commit('foo2')
+    ).then(red => {


Fixed (here and the test I copied it from)

thebrianchen · 2020-01-15T01:06:50Z

dev/test/transaction.ts

    return expect(
      runTransaction(
        () => {
          return Promise.reject('request exception');
        },
        begin(),
-        rollback('foo')
+        rollback()


should we make it consistent throughout elsewhere

thebrianchen · 2020-01-15T01:07:54Z

dev/test/transaction.ts

+          await transaction.get(docRef);
+          return 'failure';
+        },
+        begin('foo'),


If 'foo' is default, do we need it here as well, or can it also be removed?

I left it as is here since getDocument uses the second argument. I prefer this over only specifying the transaction ID once or using undefined in its place.

schmidt-sebastian

Feedback addressed. Thanks for the speedy review.

schmidt-sebastian · 2020-01-15T04:37:24Z

dev/src/transaction.ts

@@ -552,3 +561,14 @@ function validateReadOptions(
    }
  }
 }
+
+function isRetryableTransactionError(error: Error): boolean {
+  if (error instanceof GoogleError || 'code' in error) {


Yes, that is JavaScript functionality (see https://masteringjs.io/tutorials/fundamentals/hasownproperty for a comparison against Object.hasOwnProperty). I am doing it here because I am afraid that some of our dependencies might throw a GoogleError that is not actually a GoogleError by providing just a code and a message field.

schmidt-sebastian · 2020-01-15T04:37:33Z

dev/test/transaction.ts

@@ -106,7 +106,7 @@ function commit(

 function rollback(
  transaction?: Uint8Array | string,
-  err?: Error
+  error?: Error


Done. Good catch.

schmidt-sebastian · 2020-01-15T04:37:48Z

dev/test/transaction.ts

+      begin('foo2', 'foo1'),
+      getDocument('foo2'),
+      commit('foo2')
+    ).then(red => {


Fixed (here and the test I copied it from)

schmidt-sebastian · 2020-01-15T04:38:39Z

dev/test/transaction.ts

+          await transaction.get(docRef);
+          return 'failure';
+        },
+        begin('foo'),


I left it as is here since getDocument uses the second argument. I prefer this over only specifying the transaction ID once or using undefined in its place.

schmidt-sebastian · 2020-01-15T04:38:45Z

dev/test/transaction.ts

    return expect(
      runTransaction(
        () => {
          return Promise.reject('request exception');
        },
        begin(),
-        rollback('foo')
+        rollback()


…ore into mrschmidt/abort

novemberborn · 2020-01-15T09:13:19Z

dev/src/transaction.ts

+  if (error instanceof GoogleError || 'code' in error) {
+    // In transactions, the backend returns code ABORTED for reads that fail
+    // with contention. These errors should be retried for both GoogleError
+    // and GoogleError-alike errors (in case the prototype hierarchy gets


Could this check for error.name === 'GoogleError' instead?

When implementing gRPC servers, it's not unthinkable that an ABORTED error is thrown from inside the transaction, with the expectation that it's returned to the client.

This should accept any object that has a "code" property. In the environment you are describing, will this property be stripped?

Let me rephrase.

There are many ways of wrongly implementing a transaction function, to do with side-effects, or forgetting to use the transaction itself to perform queries.

This new ABORTED handling adds more ways to get it wrong.

There is an assumption that all GoogleError instances came from transaction methods. That may not be the case if I use another GCP SDK within my function. I know, I shouldn't be, but somebody still might.

It also won't be the case if I call a Firestore method on a DocumentReference which then throws an ABORTED error.

These assumptions are then generalized by testing if the thrown error has a .code = 10 property. I have a ServiceError object that is thrown with a gRPC response code. If for some reason I throw that error inside a transaction function, my transaction may be retried even if it succeeded. And again, sure, I suppose I shouldn't be throwing that error, but somebody still might.

A safer approach may be to register errors, thrown as part of the actual transaction, in a WeakMap. Then as they propagate through user code, when you catch them again you can check whether they're genuine transaction errors, and then check their code.

A couple things to note:

A read outside of a transaction should never result in Code ABORTED. This error should only happen during a transactional read.

If a user throws an error with code: 10, we will retry the transaction. In general, transaction functions should be written so they can be re-run, and re-running them more often than needed should not cause any harm.

I understand that we control the errors that we are catching here. We could tighten this check to make sure that the error is indeed a GoogleError, but for now I decided to err on the side of caution and also retry GoogleError-alike errors. This allows us to catch errors with code properties that were created by our dependencies before we converted everything to use the proper GoogleError type.

fix: manually retry ABORTED reads in transactions

e833a0d

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 14, 2020

fix wording

2c66aba

schmidt-sebastian requested a review from thebrianchen January 14, 2020 23:38

schmidt-sebastian assigned thebrianchen Jan 14, 2020

Merge branch 'master' into mrschmidt/abort

5e930d5

thebrianchen approved these changes Jan 15, 2020

View reviewed changes

Review comments

43bc22e

schmidt-sebastian commented Jan 15, 2020

View reviewed changes

Merge branch 'mrschmidt/abort' of github.com:googleapis/nodejs-firest…

06c482e

…ore into mrschmidt/abort

schmidt-sebastian merged commit 7562033 into master Jan 15, 2020

schmidt-sebastian deleted the mrschmidt/abort branch January 15, 2020 04:44

novemberborn reviewed Jan 15, 2020

View reviewed changes

yoshi-automation mentioned this pull request Jan 15, 2020

chore: re-order JSON imports #888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: manually retry ABORTED reads in transactions #883

fix: manually retry ABORTED reads in transactions #883

schmidt-sebastian commented Jan 14, 2020

codecov bot commented Jan 14, 2020 •

edited

Loading

thebrianchen left a comment

thebrianchen Jan 15, 2020

schmidt-sebastian Jan 15, 2020

thebrianchen Jan 15, 2020

schmidt-sebastian Jan 15, 2020

thebrianchen Jan 15, 2020

schmidt-sebastian Jan 15, 2020

thebrianchen Jan 15, 2020

schmidt-sebastian Jan 15, 2020

thebrianchen Jan 15, 2020

schmidt-sebastian Jan 15, 2020

schmidt-sebastian left a comment

schmidt-sebastian Jan 15, 2020

schmidt-sebastian Jan 15, 2020

schmidt-sebastian Jan 15, 2020

schmidt-sebastian Jan 15, 2020

schmidt-sebastian Jan 15, 2020

novemberborn Jan 15, 2020

schmidt-sebastian Jan 15, 2020

novemberborn Jan 16, 2020

schmidt-sebastian Jan 16, 2020

fix: manually retry ABORTED reads in transactions #883

fix: manually retry ABORTED reads in transactions #883

Conversation

schmidt-sebastian commented Jan 14, 2020

codecov bot commented Jan 14, 2020 • edited Loading

Codecov Report

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2020 •

edited

Loading