Refactor AsyncQueue's "delayed scheduling" support and add cancellation. #489

mikelehen · 2018-02-05T20:43:15Z

Introduces a DelayedOperation helper class in AsyncQueue to encapsulate
delayed op logic.
Adds cancellation support which I want to use in
Add timeout to OnlineState tracking. #412
Remove delayedOperationsCount in favor of keeping delayedOperations populated
correctly at all times.
Fixes a preexisting issue in AsyncQueue.schedule() where the returned promise
would always be resolved with undefined, instead of the result of your op.
Also remove an AnyDuringMigration usage.

* Introduces a DelayedOperation helper class in AsyncQueue to encapsulate delayed op logic. * Adds cancellation support which I want to use in #412 * Remove delayedOperationsCount in favor of keeping delayedOperations populated correctly at all times. * Fixes a preexisting issue in AsyncQueue.schedule() where the returned promise would always be resolved with undefined, instead of the result of your op. Also remove an AnyDuringMigration usage.

schmidt-sebastian · 2018-02-05T23:00:26Z

packages/firestore/src/remote/persistent_stream.ts

@@ -251,10 +251,10 @@ export abstract class PersistentStream<
  markIdle(): void {
    this.idle = true;
    this.queue
-      .schedule(() => {
+      .scheduleWithDelay(() => {
        return this.handleIdleCloseTimer();


handleIdleCloseTimer has its own logic to prevent itself from running. Do you think you could migrate this code to use this new functionality? We already cancel the timer in Android: https://cs.corp.google.com/piper///depot/google3/java/com/google/android/gmscore/dev/thick_client/firebase-firestore/src/com/google/firebase/firestore/remote/AbstractStream.java?rcl=184463491&l=362

Yeah, I noticed that. I was hoping to get away without growing this PR, but sure. :-) Please double-check that I did it right. I basically ported the Android logic.

schmidt-sebastian · 2018-02-05T23:03:13Z

packages/firestore/src/remote/persistent_stream.ts

        return this.handleIdleCloseTimer();
      }, IDLE_TIMEOUT_MS)
-      .catch((err: FirestoreError) => {
+      .promise.catch((err: FirestoreError) => {


Would it be possible to do something along the lines of https://github.com/alkemics/CancelablePromise/blob/master/CancelablePromise.js?

We might just want to expose .catch/.then, and keep .promise to make more advanced operations possible.

Good idea, done.

schmidt-sebastian · 2018-02-05T23:04:07Z

packages/firestore/src/util/async_queue.ts

-type DelayedOperation<T> = {
+/** External Result of scheduling a delayed operation. */
+export interface DelayedOperationResult<T> {
+  /** A promise that will resolve once the operation has been run. */


... and will get rejected when it is cancelled.

this went away.

schmidt-sebastian · 2018-02-05T23:08:40Z

packages/firestore/src/util/async_queue.ts

+  private timerHandle: any;
+  private readonly deferred = new Deferred<T>();
+  /** true if the operation has not been executed or cancelled yet. */
+  private pending = true;


The separation between pending and timerHandle reads nicer (because of the variable names), but isn't strictly necessary. We could just rely on timerHandle and check whether it is null or not.

Yeah. I intentionally didn't do that, but I don't feel strongly, so I dropped 'pending'.

schmidt-sebastian · 2018-02-05T23:09:26Z

packages/firestore/src/util/async_queue.ts

+    }
+  }
+
+  private runIfNecessary(): Promise<void> {


Suggest to use runIfPending to match the names in the rest of this class.

Merged this method into scheduleNow (which got renamed to handleDelayElapesd)

schmidt-sebastian · 2018-02-05T23:16:24Z

packages/firestore/src/util/async_queue.ts

+    const index = this.delayedOperations.push(delayedOp);
+
+    delayedOp.promise.catch(err => {}).then(() => {
+      this.delayedOperations.slice(index, 1);


This doesn't seem right. The index might have been changed if another promise got fulfilled before this one.

Yeesh. :-( Good catch, thank you!

schmidt-sebastian · 2018-02-05T23:17:59Z

packages/firestore/src/util/async_queue.ts

@@ -143,26 +210,14 @@ export class AsyncQueue {
   * scheduled with a delay can be rejected or queued for immediate execution.
   */
  drain(executeDelayedTasks: boolean): Promise<void> {


As a future cleanup, it seems like we could now get rid of this method. Each caller could invalidate its tasks when we call .shutdown().

Not sure how that works with the executeDelayedTasks codepath... which I think we'll actually want to extend to be able to test timeouts better...

wilhuff

This makes my brain hurt. I've tried to leave constructive feedback on how you might consider making fewer callbacks but I'm not sure that advice would actually work. Take it with a grain of salt.

wilhuff · 2018-02-05T23:05:42Z

packages/firestore/src/util/async_queue.ts

@@ -20,30 +20,103 @@ import { AnyDuringMigration, AnyJs } from './misc';
 import { Deferred } from './promise';
 import { Code, FirestoreError } from './error';

-type DelayedOperation<T> = {
+/** External Result of scheduling a delayed operation. */
+export interface DelayedOperationResult<T> {


Is there any reason to split the interface and the class?

Yes (the interface is exposed outside AsyncQueue, while the implementation is not), but this went away in favor of Sebastian's CancelablePromise suggestion.

wilhuff · 2018-02-05T23:29:02Z

packages/firestore/src/util/async_queue.ts

+  /** true if the operation has not been executed or cancelled yet. */
+  private pending = true;
+
+  static createAndSchedule<T>(


This seems unnecessarily circuitous:

scheduleWithDelay calls

createAndSchedule

which after a while calls scheduleNow(asyncQueue)

which calls asyncQueue.schedule

It mixes state in closures and in the classes in ways that are pretty confusing.

Relatedly you have createAndSchedule and scheduleNow as two vastly different operations which just from their names I would surmise as having similar behavior.

I'd suggest the following changes:

Make the asyncQueue explicitly a member of DelayedOperation

Rename scheduleNow to conflict less with createAndSchedule. Maybe submit or resume?

I'd consider just splitting creation of the op from the deferred scheduling so that you can get rid of createAndSchedule altogether. New up your DelayedOperation and then call delay on it.

Made asyncQueue a member.

Renamed scheduleNow() to a private handleDelayElapsed(), and added a new skipDelay() method.

I'm inclined to keep createAndSchedule() as it prevents the DelayedOperation from being in a created-but-not-yet-scheduled state. As-is, timerHandle is initially defined and then becomes null once the op has been executed or canceled. I'd rather not have it go null=>defined=>null with null meaning two different things. The other option is to have the constructor automatically schedule the op, but I don't really like constructors with side-effects.

By the way, part of the original naming problem is that AsyncQueue.schedule() is a bad name. It should be AsyncQueue.enqueue(), and then scheduleNow() could be enqueueNow(). But I don't want to do that in this PR.

wilhuff · 2018-02-05T23:31:23Z

packages/firestore/src/util/async_queue.ts

+   * already been run or cancelled.
+   */
+  scheduleNow(asyncQueue: AsyncQueue): void {
+    this.clearTimeout();


Would it be harmful to call clearTimeout inside runIfNecessary?

As it stands you have three different callbacks here:

The lambda that fires when setTimeout calls you back

Which calls scheduleNow

Which calls schedule with runIfNecessary.bind(this)

Done. And I merged runIfNecessary into this method.

mikelehen

PTAL.

mikelehen · 2018-02-06T00:05:06Z

packages/firestore/src/remote/persistent_stream.ts

@@ -251,10 +251,10 @@ export abstract class PersistentStream<
  markIdle(): void {
    this.idle = true;
    this.queue
-      .schedule(() => {
+      .scheduleWithDelay(() => {
        return this.handleIdleCloseTimer();


Yeah, I noticed that. I was hoping to get away without growing this PR, but sure. :-) Please double-check that I did it right. I basically ported the Android logic.

mikelehen · 2018-02-06T00:05:16Z

packages/firestore/src/remote/persistent_stream.ts

        return this.handleIdleCloseTimer();
      }, IDLE_TIMEOUT_MS)
-      .catch((err: FirestoreError) => {
+      .promise.catch((err: FirestoreError) => {


Good idea, done.

mikelehen · 2018-02-06T00:18:22Z

packages/firestore/src/util/async_queue.ts

@@ -20,30 +20,103 @@ import { AnyDuringMigration, AnyJs } from './misc';
 import { Deferred } from './promise';
 import { Code, FirestoreError } from './error';

-type DelayedOperation<T> = {
+/** External Result of scheduling a delayed operation. */
+export interface DelayedOperationResult<T> {


Yes (the interface is exposed outside AsyncQueue, while the implementation is not), but this went away in favor of Sebastian's CancelablePromise suggestion.

mikelehen · 2018-02-06T00:18:57Z

packages/firestore/src/util/async_queue.ts

-type DelayedOperation<T> = {
+/** External Result of scheduling a delayed operation. */
+export interface DelayedOperationResult<T> {
+  /** A promise that will resolve once the operation has been run. */


this went away.

mikelehen · 2018-02-06T00:37:04Z

packages/firestore/src/util/async_queue.ts

+  private timerHandle: any;
+  private readonly deferred = new Deferred<T>();
+  /** true if the operation has not been executed or cancelled yet. */
+  private pending = true;


Yeah. I intentionally didn't do that, but I don't feel strongly, so I dropped 'pending'.

mikelehen · 2018-02-06T01:02:05Z

packages/firestore/src/util/async_queue.ts

+  /** true if the operation has not been executed or cancelled yet. */
+  private pending = true;
+
+  static createAndSchedule<T>(


Made asyncQueue a member.

Renamed scheduleNow() to a private handleDelayElapsed(), and added a new skipDelay() method.

I'm inclined to keep createAndSchedule() as it prevents the DelayedOperation from being in a created-but-not-yet-scheduled state. As-is, timerHandle is initially defined and then becomes null once the op has been executed or canceled. I'd rather not have it go null=>defined=>null with null meaning two different things. The other option is to have the constructor automatically schedule the op, but I don't really like constructors with side-effects.

By the way, part of the original naming problem is that AsyncQueue.schedule() is a bad name. It should be AsyncQueue.enqueue(), and then scheduleNow() could be enqueueNow(). But I don't want to do that in this PR.

mikelehen · 2018-02-06T01:05:30Z

packages/firestore/src/util/async_queue.ts

+   * already been run or cancelled.
+   */
+  scheduleNow(asyncQueue: AsyncQueue): void {
+    this.clearTimeout();


Done. And I merged runIfNecessary into this method.

mikelehen · 2018-02-06T01:06:03Z

packages/firestore/src/util/async_queue.ts

+    }
+  }
+
+  private runIfNecessary(): Promise<void> {


Merged this method into scheduleNow (which got renamed to handleDelayElapesd)

mikelehen · 2018-02-06T01:08:41Z

packages/firestore/src/util/async_queue.ts

@@ -143,26 +210,14 @@ export class AsyncQueue {
   * scheduled with a delay can be rejected or queued for immediate execution.
   */
  drain(executeDelayedTasks: boolean): Promise<void> {


Not sure how that works with the executeDelayedTasks codepath... which I think we'll actually want to extend to be able to test timeouts better...

mikelehen · 2018-02-06T01:25:54Z

packages/firestore/src/util/async_queue.ts

+    const index = this.delayedOperations.push(delayedOp);
+
+    delayedOp.promise.catch(err => {}).then(() => {
+      this.delayedOperations.slice(index, 1);


Yeesh. :-( Good catch, thank you!

schmidt-sebastian

I can't seem to comment on Gil's suggestion and your response (maybe because it is now marked as outdated), but what do you think of this: https://gist.github.com/schmidt-sebastian/69aecafd8adffa87d369926dacf070f5

It gets rid of the asyncQueue from the DelayedOperation (this is the main cleanup)
It merges schedule and scheduleWithDelay()... but you might actually want to separate them, so please ignore if you like.
It creates two runImmediately methods on both classes that both essentially do the same thing.

schmidt-sebastian · 2018-02-06T02:09:59Z

packages/firestore/src/util/async_queue.ts

+    asyncQueue: AsyncQueue,
+    op: () => Promise<T>,
+    delayMs: number
+  ) {


Add the return type here.

Oops, thanks. Done. And opened a bug to make tslint check this for us: b/72988931

schmidt-sebastian · 2018-02-06T02:22:26Z

packages/firestore/src/remote/persistent_stream.ts

-        return this.handleIdleCloseTimer();
-      }, IDLE_TIMEOUT_MS)
-      .catch((err: FirestoreError) => {
+    if (this.isOpen() && this.inactivityTimerPromise === null) {


Might as well copy the remaining comment as well:

// Starts the idle time if we are in state 'Open' and are not yet already running a timer (in // which case the previous idle timeout still applies).

Woops, thanks!

mikelehen

Thanks for taking a stab at reworking this! I borrowed your start() helper method, as I think that's cleaner.

But the core of your change breaks a guarantee I'm trying to provide which is that if you call cancel() then the operation won't be run. With your change that's no longer true because it just cancels it from getting added to the AsyncQueue, and so if the timer has already elapsed (so it's been added to the AsyncQueue) and you call cancel(), the cancel() won't do anything and the operation will still end up being run.

Along those lines, "runImmediately()" is misleading since it only queues the operation and there may be lots of prior operations ahead of it... so it may not be immediate at all.

mikelehen · 2018-02-06T15:08:14Z

packages/firestore/src/remote/persistent_stream.ts

-        return this.handleIdleCloseTimer();
-      }, IDLE_TIMEOUT_MS)
-      .catch((err: FirestoreError) => {
+    if (this.isOpen() && this.inactivityTimerPromise === null) {


Woops, thanks!

mikelehen · 2018-02-06T15:13:28Z

packages/firestore/src/util/async_queue.ts

+    asyncQueue: AsyncQueue,
+    op: () => Promise<T>,
+    delayMs: number
+  ) {


Oops, thanks. Done. And opened a bug to make tslint check this for us: b/72988931

schmidt-sebastian

LGTM from my end.

wilhuff

LGTM

mikelehen assigned schmidt-sebastian Feb 5, 2018

mikelehen requested review from schmidt-sebastian and wilhuff as code owners February 5, 2018 20:43

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

schmidt-sebastian reviewed Feb 5, 2018

View reviewed changes

wilhuff reviewed Feb 5, 2018

View reviewed changes

CR Feedback.

3fce5f7

mikelehen commented Feb 6, 2018

View reviewed changes

schmidt-sebastian reviewed Feb 6, 2018

View reviewed changes

CR feedback.

69b4f1e

mikelehen commented Feb 6, 2018

View reviewed changes

schmidt-sebastian approved these changes Feb 6, 2018

View reviewed changes

wilhuff added api: firestore and removed needs-triage labels Feb 6, 2018

wilhuff approved these changes Feb 6, 2018

View reviewed changes

mikelehen merged commit a1e346f into master Feb 6, 2018

mikelehen deleted the mikelehen/cancellable-async-queue-tasks branch February 6, 2018 20:20

firebase locked and limited conversation to collaborators Oct 23, 2019

Refactor AsyncQueue's "delayed scheduling" support and add cancellation. #489

Refactor AsyncQueue's "delayed scheduling" support and add cancellation. #489

Conversation

mikelehen commented Feb 5, 2018

schmidt-sebastian Feb 5, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff 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

mikelehen 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

mikelehen left a 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

wilhuff left a comment

Choose a reason for hiding this comment

schmidt-sebastian Feb 5, 2018 •

edited

Loading