Closing the write and the watch stream after 60s of idleness #275

schmidt-sebastian · 2017-10-27T22:34:24Z

This is a port of firebase/firebase-ios-sdk#388

jshcrowthe · 2017-10-30T20:53:15Z

packages/firestore/test/integration/util/helpers.ts

@@ -23,6 +23,9 @@ import firebase from './firebase_export';
 import { EmptyCredentialsProvider } from '../../../src/api/credentials';
 import { PlatformSupport } from '../../../src/platform/platform';
 import { AsyncQueue } from '../../../src/util/async_queue';
+import { Firestore, FirestoreDatabase } from '../../../src/api/database';
+import { FirebaseApp } from '../../../../app/index';


This will break, if you are trying to reference other modules you need to do it via the module names

i.e.

import { FirebaseApp } from '@firebase/app';

Done. Thanks for debugging this!

wilhuff

Mostly LGTM

wilhuff · 2017-10-30T17:24:18Z

packages/firestore/src/api/database.ts

@@ -455,6 +455,11 @@ export class Firestore implements firestore.Firestore, FirebaseService {
        );
    }
  }
+
+  /** Creates a new AsyncQueue. Can be overwritten to provide a custom queue. */


s/overwritten/overridden/

wilhuff · 2017-10-30T17:24:41Z

packages/firestore/src/local/local_store.ts

-   *  or null if empty.
-   *  @param afterBatchId If provided, the batch to search after.
-   *  @returns The next mutation or null if there wasn't one.
+   * or null if empty.


Thank you for fixing these things up.

wilhuff · 2017-10-30T17:28:21Z

packages/firestore/src/remote/persistent_stream.ts

+        return this.handleIdleCloseTimer();
+      }, IDLE_TIMEOUT_MS)
+      .catch((err: FirestoreError) => {
+        // We ignore Promise rejections for cancelled idle checks.


This comment doesn't line up with the actual behavior. We don't ignore the rejection: we assert that it can only be of one type. A better comment here would articulate why we're able to make this assertion, perhaps because this can't fail except via the timeout and that's guaranteed to have Code.CANCELLED?

The reason I added this is that running the tests with this spewed a bunch of warnings in Chrome about unrejected Promises that were not handled. I made this comment better.

wilhuff · 2017-10-30T21:09:37Z

packages/firestore/src/remote/persistent_stream.ts

+  /**
+   * Closes the stream and cleans up as necessary:
+   *
+   * <ul>


Nit: Typescript comments aren't thinly veiled HTML the way Javadoc comments are. Do this as a markdown list, like we did in mutation.ts.

Adjusted. Thanks for pointing this out.

wilhuff · 2017-10-30T21:18:56Z

packages/firestore/src/remote/persistent_stream.ts

+    error?: FirestoreError
+  ): Promise<void> {
+    assert(
+      finalState == PersistentStreamState.Error || isNullOrUndefined(error),


As in the iOS PR, this allows the error to be nil if the finalState is FSTStreamStateError. Is that desirable?

This gets invoked by 'handleStreamClose' (the GRPC callback handler) and making this stricter would require that the server only ever closes a stream with an error.

wilhuff · 2017-10-30T21:23:05Z

packages/firestore/src/util/async_queue.ts

      const deferred = new Deferred<T>();
-      setTimeout(() => {
+      let handle = window.setTimeout(() => {


Why window.setTimeout() instead of the global? This is incompatible with node without extra hacks to define window.

setTimeout() is defined to (perhaps incorrectly) return a Timer. See microsoft/TypeScript#842 (comment)

wilhuff · 2017-10-30T21:23:51Z

packages/firestore/src/util/async_queue.ts

      const deferred = new Deferred<T>();
-      setTimeout(() => {
+      let handle = window.setTimeout(() => {


Also, this can be const handle instead of let handle

wilhuff · 2017-10-30T21:26:11Z

packages/firestore/src/util/async_queue.ts

+    this.delayedOperations.forEach((deferred, handle) => {
+      window.clearTimeout(handle);
+      deferred.reject(
+        new FirestoreError(Code.CANCELLED, 'Cancelled during queue drain')


These failures are potentially user visible so we need a better message about what can cause these to happen and how to avoid them if that's a problem.

If draining only happens during shutdown, the message can reflect that: 'Operation cancelled by shutdown'.

This is only used in tests, but I could see that we may want to call this during shutdown as well.

wilhuff

LGTM mod resolution of travis failures

jshcrowthe

Please remove the package-lock.json we have the top level yarn.lock that serves the same purpose.

schmidt-sebastian · 2017-11-02T23:51:03Z

@wilhuff I fixed the failing integration tests. The types we referred to in the tests ended up being minified, while the Public API that we checked against was not. I migrated the test to use the internal representation of the Public API. You can see a test run here:
https://gist.github.com/schmidt-sebastian/18116a7c92ffbfdf1d535720850e63e8

- @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected] - [email protected] - @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected]

…ment (#279) * Adds a demo app to help test and facilitate Firebase Auth API development.

Fixes verifyAssertion unrecoverable errors when returnIdpCredential is set to true. In this case, the error code is returned along with the credential in the errorMessage without any ID token/refresh token. Catch, suppress/handle when localStorage is null or when `localStorage.getItem` throws a security error due to access being disabled by the browser for whatever reason.

googlebot · 2017-11-03T22:37:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

schmidt-sebastian · 2017-11-03T22:52:37Z

@mikelehen I addressed your feedback.

The drain() method in the AsyncQueue now optionally allows for immediate execution of delayed tasks. Furthermore, I have exposed this queue via firestore.INTERNAL. The tests can now access the public Firestore client and no longer need to rely on an internal client that exposes the queue.

mikelehen · 2017-11-03T23:05:01Z

packages/firestore/src/util/async_queue.ts

+  // may also contain entries that have already been run (in which case `handle` is
+  // null).
+  //
+  // tslint:disable-next-line:no-any Accept any type of delayed operation.


Can you use AnyJs instead of "any" ?

mikelehen · 2017-11-03T23:16:09Z

packages/firestore/src/util/async_queue.ts

-        this.delayedOpCount--; // decrement once it's actually queued.
+        this.delayedOperationsCount--;
+        if (this.delayedOperationsCount > 0) {
+          delayedOp.handle = null;


nit but I'm not a fan of this sort of optimization. It saves an assignment in some cases, but reads much less naturally (to me at least). I'd just do:

delayedOp.handle = null; if (this.delayedOperationsCount === 0) { this.delayedOperations = []; }

If you insist on optimizing this sort of thing, you should go ahead and optimize away the decrement too!

if (this.delayedOperationsCount !== 1) { this.delayedOperationsCount--; delayedOp.handle = null; } else { this.delayedOperations = []; }

(please don't actually do that :-))

This wasn't meant to be the newest and greatest performance optimization. Rather, it just felt naturally to do this at the time. Your version is a bit more concise, so I went with that. Thanks.

mikelehen · 2017-11-03T23:17:35Z

packages/firestore/src/util/async_queue.ts

-      const deferred = new Deferred<T>();
-      setTimeout(() => {
+      this.delayedOperationsCount++;
+      const nextIndex = this.delayedOperations.length;


I think you can move this down and do:

const delayedOpIndex = this.delayedOperations.push(delayedOp);

I don't actually need the index anymore and removed it altogether.

mikelehen · 2017-11-03T23:19:09Z

packages/firestore/src/util/async_queue.ts

-    return this.schedule(() => Promise.resolve(undefined));
+  /**
+   * Waits until all currently scheduled tasks are finished executing. Tasks
+   * schedule with a delay can be rejected or queued for immediate execution.


schedule => scheduled

mikelehen · 2017-11-03T23:29:51Z

packages/firestore/src/api/database.ts

@@ -374,7 +375,8 @@ export class Firestore implements firestore.Firestore, FirebaseService {
    },
    // Exposed via INTERNAL for use in tests.
    disableNetwork: () => this._firestoreClient.disableNetwork(),
-    enableNetwork: () => this._firestoreClient.enableNetwork()
+    enableNetwork: () => this._firestoreClient.enableNetwork(),
+    queue: this._queue


I'm not a big fan of exposing the whole queue externally since:

Although not documented or supported, this is now exposed to consumers of the SDK and could be abused.

The only reason this works is because our minification sucks right now and we only minify things that begin or end with _. Really, the entire AsyncQueue class should be minified and all of the methods should have obfuscated names.

In general, I'd rather we make the testable API explicit and minimal. Exposing more than we need to leads to a slippery slope heading towards undesirable tight coupling between tests and implementation.

So I'd recommend exposing drainAsyncQueue() or similar (FWIW if you wanted you could make drain() always execute delayed tasks; I don't think it needs to be optional) and either remove the delayedOperationsCount assertions from tests or have the exposed method return the count of operations that were drained or something.

I went with drainAsyncTasks. In the integration tests that use Firestore, I can indeed always execute the delayed tasks. I can't do this in the spec tests (since that always ends up closing the stream and confuses the tests).

The stream tests use Datastore directly and I allowed the queue to be passed in. That way, I can verify that I have at least one pending task.

mikelehen

Looks good. Thanks!

schmidt-sebastian added 10 commits October 24, 2017 14:33

Creating Stream Tests for the Web Client

a30f63b

Updating comment

d47f81f

Using equal everywhere

5d4fa69

Addressing review feedback

d0d40ef

[AUTOMATED]: Prettier Code Styling

4dde0a2

Addressing feedback

68b04ab

Merge branch 'master' into mrschmidt-closewhenidle

035a46b

Closing stream when idle

27c5482

Closing the write and the watch stream when idle

2d5812e

[AUTOMATED]: Prettier Code Styling

8ad4df2

schmidt-sebastian requested review from mikelehen and wilhuff as code owners October 27, 2017 22:34

google-oss-bot added the needs-triage label Oct 27, 2017

schmidt-sebastian assigned wilhuff and mikelehen Oct 27, 2017

schmidt-sebastian removed the needs-triage label Oct 27, 2017

Comment fixes

a8a7d76

schmidt-sebastian force-pushed the mrschmidt-closewhenidle branch from b744b9d to a8a7d76 Compare October 28, 2017 01:16

wilhuff added the api: firestore label Oct 30, 2017

jshcrowthe suggested changes Oct 30, 2017

View reviewed changes

wilhuff reviewed Oct 30, 2017

View reviewed changes

Addressing comments & fixing build

9b1c7bb

wilhuff approved these changes Oct 31, 2017

View reviewed changes

jshcrowthe suggested changes Oct 31, 2017

View reviewed changes

jshcrowthe approved these changes Oct 31, 2017

View reviewed changes

schmidt-sebastian force-pushed the mrschmidt-closewhenidle branch 3 times, most recently from cf8f359 to 15c781c Compare November 1, 2017 18:30

schmidt-sebastian force-pushed the mrschmidt-closewhenidle branch from fa311b9 to 49ae902 Compare November 2, 2017 23:55

schmidt-sebastian and others added 11 commits November 3, 2017 14:10

Removing Package Lock

29904bb

Using a plain array to store the handles from setTimeout()

4a73968

Add a debug script to integration/firestore (#286)

34e25a6

Adding new auth code owner (#288)

18c0e6b

Update publish scripts

808878e

Publish

f24cd30

- @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected] - [email protected] - @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected] - @firebase/[email protected]

Disable automatic integration tests (#289)

f1f6611

Adds a demo app to help test and facilitate Firebase Auth API develop…

413e296

…ment (#279) * Adds a demo app to help test and facilitate Firebase Auth API development.

Removing TestFirestore and TestQueue in favor of drain()

f623c0e

[AUTOMATED]: Prettier Code Styling

3e3f205

schmidt-sebastian force-pushed the mrschmidt-closewhenidle branch from 46dd4a5 to 3e3f205 Compare November 3, 2017 22:37

schmidt-sebastian requested review from bojeil-google, gauntface, sphippen, TMSCH and wti806 as code owners November 3, 2017 22:37

schmidt-sebastian added 2 commits November 3, 2017 15:40

Merge branch 'master' into mrschmidt-closewhenidle

2951a18

Cleanup

84c6938

schmidt-sebastian force-pushed the mrschmidt-closewhenidle branch 2 times, most recently from b209f80 to 84c6938 Compare November 3, 2017 23:30

mikelehen suggested changes Nov 3, 2017

View reviewed changes

schmidt-sebastian added 2 commits November 3, 2017 17:07

Addressing feedback

255abce

[AUTOMATED]: Prettier Code Styling

7ba8832

mikelehen approved these changes Nov 4, 2017

View reviewed changes

schmidt-sebastian merged commit d43d461 into master Nov 4, 2017

schmidt-sebastian deleted the mrschmidt-closewhenidle branch November 4, 2017 20:38

firebase locked and limited conversation to collaborators Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closing the write and the watch stream after 60s of idleness #275

Closing the write and the watch stream after 60s of idleness #275

schmidt-sebastian commented Oct 27, 2017

jshcrowthe Oct 30, 2017 •

edited

Loading

schmidt-sebastian Oct 30, 2017

wilhuff left a comment

wilhuff Oct 30, 2017

schmidt-sebastian Oct 30, 2017

wilhuff Oct 30, 2017

wilhuff Oct 30, 2017

schmidt-sebastian Oct 30, 2017 •

edited

Loading

wilhuff Oct 30, 2017

schmidt-sebastian Oct 30, 2017

wilhuff Oct 30, 2017

schmidt-sebastian Oct 30, 2017

wilhuff Oct 30, 2017

schmidt-sebastian Oct 30, 2017

wilhuff Oct 30, 2017

schmidt-sebastian Oct 30, 2017

wilhuff Oct 30, 2017

schmidt-sebastian Oct 30, 2017

wilhuff left a comment •

edited

Loading

jshcrowthe left a comment •

edited

Loading

schmidt-sebastian commented Nov 2, 2017

googlebot commented Nov 3, 2017

schmidt-sebastian commented Nov 3, 2017

mikelehen Nov 3, 2017

schmidt-sebastian Nov 4, 2017

mikelehen Nov 3, 2017

schmidt-sebastian Nov 4, 2017

mikelehen Nov 3, 2017

schmidt-sebastian Nov 4, 2017

mikelehen Nov 3, 2017

schmidt-sebastian Nov 4, 2017

mikelehen Nov 3, 2017

schmidt-sebastian Nov 4, 2017

mikelehen left a comment

Closing the write and the watch stream after 60s of idleness #275

Closing the write and the watch stream after 60s of idleness #275

Conversation

schmidt-sebastian commented Oct 27, 2017

jshcrowthe Oct 30, 2017 • edited Loading

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

schmidt-sebastian Oct 30, 2017 • 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

wilhuff left a comment • edited Loading

Choose a reason for hiding this comment

jshcrowthe left a comment • edited Loading

Choose a reason for hiding this comment

schmidt-sebastian commented Nov 2, 2017

googlebot commented Nov 3, 2017

schmidt-sebastian commented Nov 3, 2017

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

mikelehen left a comment

Choose a reason for hiding this comment

jshcrowthe Oct 30, 2017 •

edited

Loading

schmidt-sebastian Oct 30, 2017 •

edited

Loading

wilhuff left a comment •

edited

Loading

jshcrowthe left a comment •

edited

Loading