Improve usage and testing of delayed operations. #499

mikelehen · 2018-02-08T17:30:51Z

Core changes:

Moves ExponentialBackoff to the AsyncQueue (matches iOS / Android).
Adds a TimerId enum for identifying delayed operations on the queue and uses it to identify our existing backoff and idle timers.
Added AsyncQueue.hasDelayedOperation(id) and .runDelayedOperationsEarly(id) which can be used from tests to check for the presence of an operation and to schedule them to run early.
- Idle tests now use these mechanisms.
- Spec tests now use this rather than setting initalBackoffDelay to 1ms.
Reworked mechanism by which DelayedOperation objects get removed from AsyncQueue's delayedOperations list to make sure it happens synchronously.

Cleanup:

Renamed schedule() to enqueue() and scheduleWithDelay() to enqueueAfterDelay().
Reorders AsyncQueue.enqueueAfterDelay() arguments to put operation last.

This PR doesn't yet expose timer ids and async queue manipulation to the spec tests, but I suspect that's the direction we'll want to go.

Core changes: * Moves ExponentialBackoff to the AsyncQueue (matches iOS / Android). * Adds a TimerId enum for identifying delayed operations on the queue and uses it to identify our existing backoff and idle timers. * Added AsyncQueue.hasDelayedOperation(id) and .runDelayedOperationsEarly(id) which can be used from tests to check for the presence of an operation and to schedule them to run early. * Idle tests now use these mechanisms. * Spec tests now use this rather than setting initalBackoffDelay to 1ms. * Reworked mechanism by which DelayedOperation objects get removed from AsyncQueue's delayedOperations list to make sure it happens synchronously. Cleanup: * Renamed schedule() to enqueue() and scheduleWithDelay() to enqueueAfterDelay(). * Reorders AsyncQueue.enqueueAfterDelay() arguments to put operation last.

schmidt-sebastian · 2018-02-08T18:54:19Z

packages/firestore/src/util/async_queue.ts

+ * AsyncQueue. These IDs can then be used from tests to check for the presence
+ * of operations or to run them early.
+ */
+export enum TimerId {


I'm not sure I am a big fan of this Enum. Can we use well-defined stringslike we for the action strings in Persistence.runTransaction (maybe make them of type TimeId)? It seems strange that we would have to edit this type everytime we add a new task.

I started out with a string union (type TimerId = 'listen_stream_idle'|...), but it was feeling awkward to have long string constants that had to match between our tests and implementation code. And since iOS / Java don't have string unions I assume we'd want to use an enum there? So I just went with an enum here too.

I don't follow "It seems strange that we would have to edit this type everytime we add a new task." Wouldn't the same be true if we used a string union? Or were you thinking we'd forego type-safety? That's an option, but I do actually like having all of the timers defined in one place (so if you're adding or testing timers, you can reason about the interaction with the existing ones).

FWIW, I think the runTransaction strings are just used for logging, so they don't have the same constraint of being used from multiple places.

I'm not sure I am sold my initial idea for this anymore either, but I was indeed thinking that we should forego type safety entirely. Something along those lines of

queue. enqueueAfterDelay('myamazingoperation', () => {}, ...) queue. runDelayedOperationsEarly('myamazingoperation');

feels natural to me. The argument against this is that we will never actually use our API like this... ...and that's a pretty strong argument.

If we do have a single list of acceptable values, then it seems better to stick with the enum.

schmidt-sebastian

Functionality wise, this PR looks fine to me. I wonder though if we need the guarantees it provides and if we can maybe reduce the complexity a bit by by relaxing some of them. This is a lot of code for a queue in JavaScript that essentially only wraps setTimeout.

schmidt-sebastian · 2018-02-08T19:12:32Z

packages/firestore/src/remote/backoff.ts

-  backoffAndWait(): Promise<void> {
-    const def = new Deferred<void>();
+  backoffAndRun(op: () => Promise<void>): void {
+    if (this.timerPromise !== null) {


With the old API, we had a way to tell the caller that its operation got cancelled. Right now, if I follow this correctly, we will just silently drop the previous invocation/

Not 100% sure what you're referring to. Prior to my change we never canceled backoff timers. This actually means that you could end up with multiple outstanding backoff timers. While this doesn't /break/ anything, I'm not sure it was intentional and it means we might reconnect earlier than we should.

I did change the return type of this method to void instead of Promise, but that's just because we weren't using it and it matches iOS / Android now.

Let's keep it as is for now, but I do think that it would be useful to be able to figure that when operations get cancelled. If we don't need this now, we can deal with across all platforms we end up needing it.

schmidt-sebastian · 2018-02-08T19:20:14Z

packages/firestore/src/remote/persistent_stream.ts

@@ -163,13 +163,15 @@ export abstract class PersistentStream<

  constructor(
    private queue: AsyncQueue,
+    backoffTimerId: TimerId,


This name makes it sound like you want to use different IDs for operations that get backed off. I don't think we should do that, and instead use the same ID regardless of how it gets scheduled.
Do you mind changing this to connectionTimerId (or something similar) to indicate the operation type rather than the fact that it is can be run with backoff?

Good point. Renamed. I also changed the TimerId enums (ListenStreamConnection instead of ListenStreamBackoff).

schmidt-sebastian · 2018-02-08T19:21:39Z

packages/firestore/src/remote/backoff.ts

   */
-  backoffAndWait(): Promise<void> {
-    const def = new Deferred<void>();
+  backoffAndRun(op: () => Promise<void>): void {


backoffAndRun lets you schedule any type of operation here, but the ID already got assigned in the constructor. Should we move the assignment of the ID to here as well?

Eh... The intention is that an ExponentialBackoff object is used for one purpose (restarting the listen stream for instance). It wouldn't make sense to do a different kind of operation each time backoff elapses. So I think it'd be more inline with the intent of ExponentialBackoff to move op to the constructor, but I think that makes usage of the class more awkward. So I am inclined to keep it as-is.

schmidt-sebastian · 2018-02-08T19:25:11Z

packages/firestore/src/util/async_queue.ts

-    private op: () => Promise<T>
+    private readonly asyncQueue: AsyncQueue,
+    readonly timerId: TimerId,
+    readonly targetTime: number,


TargetTime is pretty unspecific. Should me make this a Date or call this targetTimeMs?

Oops, good call. Renamed to targetTimeMs. I don't want to use Date since I use this for sorting, and ms is much easier.

schmidt-sebastian · 2018-02-08T19:25:27Z

packages/firestore/src/util/async_queue.ts

-    private asyncQueue: AsyncQueue,
-    private op: () => Promise<T>
+    private readonly asyncQueue: AsyncQueue,
+    readonly timerId: TimerId,


Should this name actually be OperationId or something similar?

I think OperationId is too generic (most operations don't have IDs). I actually named it DelayedOperationId at first, but I didn't like it. It was verbose, and I think "timer" conveys the purpose a lot better and makes all the usages clearer (idleTimerId vs idleOperationId, etc.). Conceptually, the client has various timers (for backoff, idle, etc.) and using these "timer ids", tests can now start to manipulate them...

If you feel strongly, I can rename it back to DelayedOperationId, but I'm still preferring TimerId.

schmidt-sebastian · 2018-02-08T19:28:27Z

packages/firestore/src/util/async_queue.ts

    this.delayedOperations.push(delayedOp);

-    delayedOp.catch(err => {}).then(() => {


I think this way of dealing with the dequeuing was much more straightforward (albeit you need to re-throw the error)

Yeah, the problem was it runs asynchronously and so the delayedOperations list will sometimes contain canceled or already-run operations, causing various problems (like if you cancel a task and then immediately use asyncQueue.hasDelayedOperation() to check for it, it'll erroneously return true).

It also won't port well to other platforms.

schmidt-sebastian · 2018-02-08T19:35:32Z

packages/firestore/src/util/async_queue.ts

+
+      // Run ops in the same order they'd run if they ran naturally.
+      this.delayedOperations.sort((a, b) => a.targetTime - b.targetTime);
+


This seems like a lot of hoops to go through that (at least for now) I don't cleary see the immediate benefit of. For unrelated operations scheduled with delay, it seems fine to a) run them out of order (removing the need for target time) and b) only run them when calling runDelayedOperationsEarly.

My goal is to basically give our tests control of time so you can "advance the clock" in a test and make sure the right things happen. When a test does this, I think operations should run in the same order they'll run in the real client. That way we can detect interactions between our various timeouts.

I'm not 100% sure what b) is referring to, but FWIW I don't think we want to always run all delayed operations. Right now for instance, the spec tests trigger a reconnect and drain the connection timer, but they don't want to drain the idle timer, and any other timers we add in the future.

schmidt-sebastian · 2018-02-08T19:36:35Z

packages/firestore/test/integration/api_internal/idle_timeout.test.ts

@@ -25,7 +26,9 @@ apiDescribe('Idle Timeout', persistence => {
      return docRef
        .set({ foo: 'bar' })
        .then(() => {
-          return drainAsyncQueue(db);
+          return asyncQueue(db).runDelayedOperationsEarly(
+            TimerId.WriteStreamIdle


This would also read nicer to me if this was a constant on WriteStream.

What will the type of this constant be? In Java?

I was going to suggest a String but I think you already changed my mind of that.

mikelehen

Yeah, sorry for the churn. All of this is pre-work for me adding (and testing) the new OnlineState timeout (#412).

I think this is worthwhile in order to have the ability for tests to manipulate time and verify that the right things happen.

mikelehen · 2018-02-08T20:19:25Z

packages/firestore/src/remote/backoff.ts

   */
-  backoffAndWait(): Promise<void> {
-    const def = new Deferred<void>();
+  backoffAndRun(op: () => Promise<void>): void {


Eh... The intention is that an ExponentialBackoff object is used for one purpose (restarting the listen stream for instance). It wouldn't make sense to do a different kind of operation each time backoff elapses. So I think it'd be more inline with the intent of ExponentialBackoff to move op to the constructor, but I think that makes usage of the class more awkward. So I am inclined to keep it as-is.

mikelehen · 2018-02-08T20:26:27Z

packages/firestore/src/remote/backoff.ts

-  backoffAndWait(): Promise<void> {
-    const def = new Deferred<void>();
+  backoffAndRun(op: () => Promise<void>): void {
+    if (this.timerPromise !== null) {


Not 100% sure what you're referring to. Prior to my change we never canceled backoff timers. This actually means that you could end up with multiple outstanding backoff timers. While this doesn't /break/ anything, I'm not sure it was intentional and it means we might reconnect earlier than we should.

I did change the return type of this method to void instead of Promise, but that's just because we weren't using it and it matches iOS / Android now.

mikelehen · 2018-02-08T20:27:52Z

packages/firestore/src/remote/persistent_stream.ts

@@ -163,13 +163,15 @@ export abstract class PersistentStream<

  constructor(
    private queue: AsyncQueue,
+    backoffTimerId: TimerId,


Good point. Renamed. I also changed the TimerId enums (ListenStreamConnection instead of ListenStreamBackoff).

mikelehen · 2018-02-08T22:33:27Z

packages/firestore/src/util/async_queue.ts

-    private asyncQueue: AsyncQueue,
-    private op: () => Promise<T>
+    private readonly asyncQueue: AsyncQueue,
+    readonly timerId: TimerId,


I think OperationId is too generic (most operations don't have IDs). I actually named it DelayedOperationId at first, but I didn't like it. It was verbose, and I think "timer" conveys the purpose a lot better and makes all the usages clearer (idleTimerId vs idleOperationId, etc.). Conceptually, the client has various timers (for backoff, idle, etc.) and using these "timer ids", tests can now start to manipulate them...

If you feel strongly, I can rename it back to DelayedOperationId, but I'm still preferring TimerId.

mikelehen · 2018-02-08T22:34:35Z

packages/firestore/src/util/async_queue.ts

-    private op: () => Promise<T>
+    private readonly asyncQueue: AsyncQueue,
+    readonly timerId: TimerId,
+    readonly targetTime: number,


Oops, good call. Renamed to targetTimeMs. I don't want to use Date since I use this for sorting, and ms is much easier.

mikelehen · 2018-02-08T22:39:30Z

packages/firestore/src/util/async_queue.ts

    this.delayedOperations.push(delayedOp);

-    delayedOp.catch(err => {}).then(() => {


Yeah, the problem was it runs asynchronously and so the delayedOperations list will sometimes contain canceled or already-run operations, causing various problems (like if you cancel a task and then immediately use asyncQueue.hasDelayedOperation() to check for it, it'll erroneously return true).

It also won't port well to other platforms.

mikelehen · 2018-02-08T22:50:34Z

packages/firestore/src/util/async_queue.ts

+
+      // Run ops in the same order they'd run if they ran naturally.
+      this.delayedOperations.sort((a, b) => a.targetTime - b.targetTime);
+


My goal is to basically give our tests control of time so you can "advance the clock" in a test and make sure the right things happen. When a test does this, I think operations should run in the same order they'll run in the real client. That way we can detect interactions between our various timeouts.

I'm not 100% sure what b) is referring to, but FWIW I don't think we want to always run all delayed operations. Right now for instance, the spec tests trigger a reconnect and drain the connection timer, but they don't want to drain the idle timer, and any other timers we add in the future.

mikelehen · 2018-02-08T22:51:12Z

packages/firestore/test/integration/api_internal/idle_timeout.test.ts

@@ -25,7 +26,9 @@ apiDescribe('Idle Timeout', persistence => {
      return docRef
        .set({ foo: 'bar' })
        .then(() => {
-          return drainAsyncQueue(db);
+          return asyncQueue(db).runDelayedOperationsEarly(
+            TimerId.WriteStreamIdle


What will the type of this constant be? In Java?

schmidt-sebastian

This looks fine to me (after some convincing), but I do still wonder if this could be simpler overall.

schmidt-sebastian · 2018-02-09T18:39:13Z

packages/firestore/src/remote/backoff.ts

-  backoffAndWait(): Promise<void> {
-    const def = new Deferred<void>();
+  backoffAndRun(op: () => Promise<void>): void {
+    if (this.timerPromise !== null) {


Let's keep it as is for now, but I do think that it would be useful to be able to figure that when operations get cancelled. If we don't need this now, we can deal with across all platforms we end up needing it.

schmidt-sebastian · 2018-02-09T18:55:20Z

packages/firestore/src/util/async_queue.ts

+ * AsyncQueue. These IDs can then be used from tests to check for the presence
+ * of operations or to run them early.
+ */
+export enum TimerId {


I'm not sure I am sold my initial idea for this anymore either, but I was indeed thinking that we should forego type safety entirely. Something along those lines of

queue. enqueueAfterDelay('myamazingoperation', () => {}, ...) queue. runDelayedOperationsEarly('myamazingoperation');

feels natural to me. The argument against this is that we will never actually use our API like this... ...and that's a pretty strong argument.

If we do have a single list of acceptable values, then it seems better to stick with the enum.

schmidt-sebastian · 2018-02-09T18:57:05Z

packages/firestore/src/util/async_queue.ts

+  ListenStreamIdle,
+  ListenStreamConnection,
+  WriteStreamIdle,
+  WriteStreamConnection


I am thinking more and more that we should combine these four states into two (StreamIdle, StreamConnection), which would simplify our stream constructors considerably.

If we do that then we have to allow duplicates on the queue and it becomes ambiguous which one tests are manipulating, so I'm inclined to keep them separate. But I agree the stream constructor ends up awkward. :-/ I considered having them be abstract properties on the stream instead (so each subclass would need to define them appropriately), but I'm not sure that's actually better, and abstract doesn't port super great to Obj-C.

This strikes me as pretty ugly too but I don't think reducing the number of states is worthwhile.

What if we fully constructed the exponential backoff and passed that in to the remote store instead of constructing it here?

Similarly can we encapsulate the idle id and duration into a "delayed runner"? That way the streams just have these dumb interfaces to delay tasks without having to distinguish how long to delay or which id to use where.

I'm not 100% sure what you're proposing. We could do something like:

Rename TimerId enum to TimeoutId

Add a Timeout tuple class (encapsulating "idle id" and "duration" as you suggested):

class Timeout { constructor(public delayMs: number, public timeoutId: TimoutId) { } }

Change
asyncQueue.enqueueAfterDelay(timerId: TimerId, delayMs: number, op: () => Promise<T>)
to: asyncQueue.enqueueAfterTimeout(timeout: Timeout, op: () => Promise<T>)

Change PersistentStream constructor to accept an ExponentialBackoff object and Timeout object:

constructor( private queue: AsyncQueue, connectionTimeout: Timeout, // or instead: connectionBackoff: ExponentialBackoff, private idleTimeout: Timeout, protected connection: Connection, private credentialsProvider: CredentialsProvider ) { ... }

I don't have strong feelings about any of this, but it doesn't reduce the number of arguments to PersistentStream's constructor.

If you're proposing we actually have a DelayedRunner class/interface that encapsulates an AsyncQueue + Timeout so we can pass it into the stream and it can do idleTimeoutRunner.run(op), we could... but going the next step and trying to make ExponentialBackoff implement DelayedRunner as well would require surgery to move all the .reset() and .resetToMax() calls out of PersistentStream and I'm not sure it is feasible or makes sense.

And at the end of the day we're still passing in an "idleBLAH" and "connectionBLAH" to the PersistentStream constructor (where "BLAH" could be "TimerId" (today) or "Timeout" or "DelayedRunner").

So based on my understanding of your suggestion, I am not really excited... but let me know if my understanding missed.

Nope, you understood what I meant. And yeah, it's not much of an improvement.

schmidt-sebastian · 2018-02-09T19:37:54Z

packages/firestore/src/util/async_queue.ts

+  /**
+   * For Tests: Determine if a particular delayed operation exists.
+   */
+  hasDelayedOperation(timerId: TimerId): boolean {


Don't feel strongly, but maybe use ContainsDelayedOperation here since has indicates a 1:1 mapping?

Sure, sounds good.

schmidt-sebastian · 2018-02-09T19:40:09Z

packages/firestore/test/integration/api_internal/idle_timeout.test.ts

@@ -25,7 +26,9 @@ apiDescribe('Idle Timeout', persistence => {
      return docRef
        .set({ foo: 'bar' })
        .then(() => {
-          return drainAsyncQueue(db);
+          return asyncQueue(db).runDelayedOperationsEarly(
+            TimerId.WriteStreamIdle


I was going to suggest a String but I think you already changed my mind of that.

mikelehen

Thanks!

mikelehen · 2018-02-09T20:13:09Z

packages/firestore/src/util/async_queue.ts

+  ListenStreamIdle,
+  ListenStreamConnection,
+  WriteStreamIdle,
+  WriteStreamConnection


If we do that then we have to allow duplicates on the queue and it becomes ambiguous which one tests are manipulating, so I'm inclined to keep them separate. But I agree the stream constructor ends up awkward. :-/ I considered having them be abstract properties on the stream instead (so each subclass would need to define them appropriately), but I'm not sure that's actually better, and abstract doesn't port super great to Obj-C.

mikelehen · 2018-02-09T20:17:32Z

packages/firestore/src/util/async_queue.ts

+  /**
+   * For Tests: Determine if a particular delayed operation exists.
+   */
+  hasDelayedOperation(timerId: TimerId): boolean {


Sure, sounds good.

mikelehen assigned schmidt-sebastian Feb 8, 2018

mikelehen requested a review from schmidt-sebastian February 8, 2018 17:30

mikelehen requested a review from wilhuff as a code owner February 8, 2018 17:30

google-oss-bot added the needs-triage label Feb 8, 2018

schmidt-sebastian reviewed Feb 8, 2018

View reviewed changes

schmidt-sebastian suggested changes Feb 8, 2018

View reviewed changes

CR Feedback.

42f16e4

mikelehen commented Feb 8, 2018

View reviewed changes

schmidt-sebastian approved these changes Feb 9, 2018

View reviewed changes

hasDelayedOperation() => containsDelayedOperation().

e889e0c

mikelehen commented Feb 9, 2018

View reviewed changes

mikelehen merged commit fce4168 into master Feb 11, 2018

mikelehen deleted the mikelehen/async-queue-delayed-task-control branch February 11, 2018 23:39

firebase locked and limited conversation to collaborators Oct 23, 2019

		this.delayedOperations.push(delayedOp);

		delayedOp.catch(err => {}).then(() => {


		// Run ops in the same order they'd run if they ran naturally.
		this.delayedOperations.sort((a, b) => a.targetTime - b.targetTime);

Improve usage and testing of delayed operations. #499

Improve usage and testing of delayed operations. #499

Uh oh!

Conversation

mikelehen commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

mikelehen Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

mikelehen commented Feb 8, 2018 •

edited

Loading

schmidt-sebastian Feb 8, 2018 •

edited

Loading

mikelehen Feb 8, 2018 •

edited

Loading

mikelehen Feb 8, 2018 •

edited

Loading

mikelehen Feb 8, 2018 •

edited

Loading

schmidt-sebastian Feb 9, 2018 •

edited

Loading