Don't delay retryable operations #3270

schmidt-sebastian · 2020-06-24T06:19:30Z

One of the behavior changes of enqueueRetryable (#2879) was that it only scheduled the next operation when the first operation succeeds. This means that any operation that was scheduled after the second retryable operation will run before this run:

enqueue(console.log(1))
enqueueRetryable(console.log(2))
enqueueRetryable(console.log(3))
enqueue(console.log(4))

-> prints 1,2,4,3 since 3 is only enqueued once 2 finishes

While theoretically okay, this still seems to cause some issues with the user change. This PR changes the retry behavior so that operations run in the original order if no failure occurred.

Addresses #3218

google-oss-bot · 2020-06-24T06:28:12Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (c436bcc) Head (aec0915) Diff

browser 248 kB 248 kB +79 B (+0.0%)

esm2017 194 kB 194 kB -16 B (-0.0%)

main 493 kB 493 kB +37 B (+0.0%)

module 246 kB 246 kB +79 B (+0.0%)
@firebase/firestore/lite

Type Base (c436bcc) Head (aec0915) Diff

main 140 kB 140 kB -53 B (-0.0%)
@firebase/firestore/memory

Type Base (c436bcc) Head (aec0915) Diff

browser 186 kB 186 kB +79 B (+0.0%)

esm2017 145 kB 145 kB -16 B (-0.0%)

main 363 kB 363 kB +37 B (+0.0%)

module 184 kB 184 kB +79 B (+0.0%)
firebase

Type Base (c436bcc) Head (aec0915) Diff

firebase-firestore.js 287 kB 287 kB +78 B (+0.0%)

firebase-firestore.memory.js 226 kB 226 kB +78 B (+0.0%)

firebase.js 820 kB 820 kB +78 B (+0.0%)

Test Logs

Base (c436bcc6): https://github.com/firebase/firebase-js-sdk/actions/runs/146607580
Head (aec09151): https://github.com/firebase/firebase-js-sdk/actions/runs/146638279

var-const

A couple of comments on the PR's description:

One of the behavior changes of enqueueRetryable was...
Can you add a link to the PR with the change?

This means that any operation that was scheduled after the second retryable operation will run before this run.

Can you rephrase and/or expand this sentence? I don't fully understand how this follows from the behavior change mentioned in the previous sentence.

var-const · 2020-06-24T19:08:01Z

packages/firestore/test/unit/util/async_queue.test.ts

@@ -313,6 +312,65 @@ describe('AsyncQueue', () => {
    expect(completedSteps).to.deep.equal([1, 1, 2]);
  });

+  it('Doesn not delay retryable operations that succeed', async () => {


Nit: s/Doesn/Does/.

Oops. Fixed.

var-const · 2020-06-24T19:09:02Z

packages/firestore/src/util/async_queue.ts

+  private async retryNextOp(): Promise<void> {
+    const op = this.retryableOps.shift();
+
+    if (op) {


Can you use an early return instead to reduce nestedness?

Updated. I also ended up rewriting this a little bit to make flow easier to follow.

var-const · 2020-06-24T19:11:45Z

packages/firestore/src/util/async_queue.ts

-    });
+      }
+
+      this.backoff.backoffAndRun(() => this.retryNextOp());


Question: why is backoff invoked in the successful case?

I added a comment.

var-const · 2020-06-24T19:12:17Z

packages/firestore/src/util/async_queue.ts

@@ -207,9 +207,9 @@ export class AsyncQueue {
  // The last promise in the queue.
  private tail: Promise<unknown> = Promise.resolve();

-  // The last retryable operation. Retryable operation are run in order and
+  // A list of retryable operations Retryable operation are run in order and


Nit: missing full stop.

var-const · 2020-06-24T19:12:52Z

packages/firestore/src/util/async_queue.ts

@@ -207,9 +207,9 @@ export class AsyncQueue {
  // The last promise in the queue.
  private tail: Promise<unknown> = Promise.resolve();

-  // The last retryable operation. Retryable operation are run in order and
+  // A list of retryable operations Retryable operation are run in order and


Nit: should be Retryable operations are run in order... (i.e. plural).

var-const · 2020-06-24T19:17:55Z

packages/firestore/test/unit/specs/recovery_spec.test.ts

@@ -787,6 +787,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
          .expectActiveTargets({ query })
          // We are now user 2
          .expectEvents(query, { removed: [doc1], fromCache: true })
+          .runTimer(TimerId.AsyncQueueRetry)


Question: why is this change necessary?

The previous implementation used "enqueue" to run the next delayed operation, this implementation uses "enqueueWithBackoff". I verified that the backoff is 0 at this step, but even operations with zero backoff aren't automatically drained in the SpecTestRunner.

var-const

Can you please take a look at the comments regarding the PR's description? Thanks!

schmidt-sebastian

PR comment updated.

Don't delay retryable operations

2187e60

schmidt-sebastian force-pushed the mrschmidt/retryloop branch from 9b371a9 to 2187e60 Compare June 24, 2020 06:23

schmidt-sebastian requested a review from var-const June 24, 2020 06:23

schmidt-sebastian assigned var-const Jun 24, 2020

schmidt-sebastian added the api: firestore label Jun 24, 2020

var-const suggested changes Jun 24, 2020

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Jun 24, 2020

Feedback

0ee1207

schmidt-sebastian force-pushed the mrschmidt/retryloop branch from 614b1d7 to 0ee1207 Compare June 24, 2020 20:45

schmidt-sebastian assigned var-const and unassigned schmidt-sebastian Jun 24, 2020

var-const assigned schmidt-sebastian and unassigned var-const Jun 24, 2020

var-const approved these changes Jun 24, 2020

View reviewed changes

schmidt-sebastian commented Jun 24, 2020

View reviewed changes

schmidt-sebastian merged commit 8846f61 into master Jun 24, 2020

schmidt-sebastian deleted the mrschmidt/retryloop branch June 24, 2020 22:12

firebase locked and limited conversation to collaborators Jul 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't delay retryable operations #3270

Don't delay retryable operations #3270

schmidt-sebastian commented Jun 24, 2020 •

edited by var-const

Loading

google-oss-bot commented Jun 24, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

var-const left a comment

var-const Jun 24, 2020

schmidt-sebastian Jun 24, 2020

var-const Jun 24, 2020

schmidt-sebastian Jun 24, 2020

var-const Jun 24, 2020

schmidt-sebastian Jun 24, 2020

var-const Jun 24, 2020

schmidt-sebastian Jun 24, 2020

var-const Jun 24, 2020

var-const Jun 24, 2020

schmidt-sebastian Jun 24, 2020

var-const left a comment

schmidt-sebastian left a comment

Don't delay retryable operations #3270

Don't delay retryable operations #3270

Conversation

schmidt-sebastian commented Jun 24, 2020 • edited by var-const Loading

google-oss-bot commented Jun 24, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/lite

@firebase/firestore/memory

firebase

Test Logs

var-const 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

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented Jun 24, 2020 •

edited by var-const

Loading

google-oss-bot commented Jun 24, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`