Don't crash if Target cannot be persisted #2944

schmidt-sebastian · 2020-04-20T05:19:52Z

This PR rejects a Query listener if its Target cannot be allocated.

Addresses #2755

google-oss-bot · 2020-04-20T05:28:14Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (e03614c) Head (99b7441) Diff

browser 247 kB 247 kB +324 B (+0.1%)

esm2017 193 kB 193 kB +112 B (+0.1%)

main 487 kB 488 kB +379 B (+0.1%)

module 245 kB 245 kB +302 B (+0.1%)
@firebase/firestore/memory

Type Base (e03614c) Head (99b7441) Diff

browser 188 kB 188 kB +324 B (+0.2%)

esm2017 148 kB 148 kB +112 B (+0.1%)

main 364 kB 365 kB +379 B (+0.1%)

module 186 kB 187 kB +302 B (+0.2%)

`firebase`

Type	Base (`e03614c`)	Head (`99b7441`)	Diff
firebase-firestore.js	289 kB	289 kB	+304 B (+0.1%)
firebase-firestore.memory.js	231 kB	231 kB	+304 B (+0.1%)
firebase.js	822 kB	823 kB	+306 B (+0.0%)

Test Logs

Base (e03614c7): https://github.com/firebase/firebase-js-sdk/actions/runs/92702128
Head (99b74415): https://github.com/firebase/firebase-js-sdk/actions/runs/92799858

schmidt-sebastian · 2020-04-20T16:22:01Z

packages/firestore/src/core/event_manager.ts

@@ -59,15 +67,14 @@ export class EventManager implements SyncEngineListener {
    this.syncEngine.subscribe(this);
  }

-  listen(listener: QueryListener): Promise<TargetId> {
+  async listen(listener: QueryListener): Promise<void> {


I removed the return value since a failed target doesn't have an ID. The only consumer of the targetId result were the spec tests, which already verify the target ID in every step as part of expectedActiveTargets.

Why doesn't failing to listen reject the promise here? If it did then you could return the TargetId in the successful case.

If I reject the Promise here, then FirestoreClient has to handle the rejected Promise. This is actually pretty nice and keeps the code in EventManager much cleaner. The calling code is also still clean, since it was simple to begin with.

The problem I see is that this makes this code non-testable. The spec tests don't use FirestoreClient, and the integration cannot inject failures. I have made the change regardless and added some special handling in SyncEngine. I am not sure if this is an improvement though.

I see what you mean; the spec test has to emulate this for itself. That's not completely awful but you're probably right that the other way was better because it was testable.

Would it help to put the error handling in SyncEngine?

I think SyncEngine is not the right place for this since it can only convey two states: Success or failure. We really need three states: Success, failure (but don't crash AsyncQueue) and failure (crash AsyncQueue).
Unless we change SyncEngine.listen() return a tristate, we have to wrap any error thrown by SyncEngine in the same if (error.name === '...') check when we invoke the QueryListener.

Given this and the test ability, I moved the code back to EventListener. It's not as clean looking, but it will probably be easier to keep regression free.

schmidt-sebastian · 2020-04-20T17:26:08Z

packages/firestore/src/core/sync_engine.ts

@@ -246,8 +252,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      }
    }

-    this.syncEngineListener!.onWatchChange([viewSnapshot]);


I had to remove this indirection. onWatchChange doesn't fire if EventManager doesn't execute this.queries.set(query, queryInfo) first.

At first I was concerned about this because it seemed like this would prevent calling onViewSnapshot on the listener, but actually it looks like code to do that was already explicitly in the EventManager (in if (queryInfo.viewSnap), and I'm guessing that the explicit code was just being treated as a no-op because previously queryInfo.viewSnap would be undefined before?

I am not sure I fully understand this, but I hope that my previous comment reliefs your concerns. Let me know if it doesn't.

schmidt-sebastian · 2020-04-20T17:28:25Z

packages/firestore/src/core/sync_engine.ts

@@ -98,6 +98,12 @@ class QueryView {
  ) {}
 }

+/** Result type returned during initial query registration. */
+export interface QueryRegistration {


We now have three different versions of this interface: QueryRegistration. ViewChange, QueryListenersInfo

They are all a bit different and i don't see a way to combine them. ViewChange would be the best candidate, but the View does not expose the targetId.

The old arrangement where the sync engine was responsible for the first snapshot avoided this problem.

I wonder if would be possible for the sync engine to call back indicating the listen failed instead? That way EventManager.listen could proceed optimistically and then just surface the error/remove the listener as normal without any changes.

The issue here is actually the success case - the problem here is the ViewSnapshot for the initial listen needs to be raised. The current flow is:

EventManager's listen() registers target in local data structure

EventManager calls SyncEngine listen()

SyncEngine's listen() calls back EventManager

EventManager looks in local data structure to raise snapshot

SyncEngine finishes listen()

EventManager finishes listen()

This code seems fragile to me The callback in SyncEngine relies on an implementation detail of EventManager. I was very surprised that re-arranging the internal code of EventManager broke tests that were unrelated to my change. Because of this, I think it is better to return a ViewSnapshot.

Having said that, I was able to remove this type after all because EventManager doesn't actually use the target ID (other than to pass it through). I updated listen() to just return the initial snapshot.

wilhuff · 2020-04-22T21:43:08Z

packages/firestore/CHANGELOG.md

@@ -1,4 +1,6 @@
 # Unreleased
+- [fixed] Firestore now rejects `onSnapshot()` listeners if they cannot be


This and the next release note could probably be combined. Something like:

Firestore now rejects onSnapshot listeners are writes if they cannot be persisted in IndexedDB. ...

The prior change is going out this week. Unfortunately, this Changelog is not all that clean, but this is mostly due to the fact the version numbers are impossible to figure out. Maybe we can discuss this with the team and clean it up once and for all.

wilhuff · 2020-04-22T21:44:08Z

packages/firestore/src/core/event_manager.ts

@@ -59,15 +67,14 @@ export class EventManager implements SyncEngineListener {
    this.syncEngine.subscribe(this);
  }

-  listen(listener: QueryListener): Promise<TargetId> {
+  async listen(listener: QueryListener): Promise<void> {


Why doesn't failing to listen reject the promise here? If it did then you could return the TargetId in the successful case.

wilhuff · 2020-04-22T22:03:03Z

packages/firestore/src/core/event_manager.ts

+        queryInfo.targetId = targetId;
+        queryInfo.viewSnap = snapshot;
+      } catch (e) {
+        if (e.name === 'IndexedDbTransactionError') {


This may not be possible, but it seems like could benefit from handling these errors consistently in our API.

For example, we try to make FirestoreClient the place where we centrally handle dispatching onto the AsyncQueue. Should we try to similarly make handling retries (or error handling like this) a part of the SyncEngine?

This doesn't seem to work in this case because you're returning a successful promise in case of a known error, but that seems to be catering to the caller's use of this function in a way that makes this more complicated than it needs to be. Maybe the caller should handle this in the block submitted to the async queue?

I did end up updating this, but as stated this pushes the error handling to both FirestoreClient and SpecTestRunner.

wilhuff · 2020-04-22T22:06:31Z

packages/firestore/src/core/sync_engine.ts

@@ -246,8 +252,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      }
    }

-    this.syncEngineListener!.onWatchChange([viewSnapshot]);


At first I was concerned about this because it seemed like this would prevent calling onViewSnapshot on the listener, but actually it looks like code to do that was already explicitly in the EventManager (in if (queryInfo.viewSnap), and I'm guessing that the explicit code was just being treated as a no-op because previously queryInfo.viewSnap would be undefined before?

wilhuff · 2020-04-22T22:13:11Z

packages/firestore/src/core/sync_engine.ts

@@ -98,6 +98,12 @@ class QueryView {
  ) {}
 }

+/** Result type returned during initial query registration. */
+export interface QueryRegistration {


The old arrangement where the sync engine was responsible for the first snapshot avoided this problem.

I wonder if would be possible for the sync engine to call back indicating the listen failed instead? That way EventManager.listen could proceed optimistically and then just surface the error/remove the listener as normal without any changes.

schmidt-sebastian

Thanks for the review. I have another controversial version for you :)

schmidt-sebastian · 2020-04-23T01:02:04Z

packages/firestore/CHANGELOG.md

@@ -1,4 +1,6 @@
 # Unreleased
+- [fixed] Firestore now rejects `onSnapshot()` listeners if they cannot be


The prior change is going out this week. Unfortunately, this Changelog is not all that clean, but this is mostly due to the fact the version numbers are impossible to figure out. Maybe we can discuss this with the team and clean it up once and for all.

schmidt-sebastian · 2020-04-23T04:09:05Z

packages/firestore/src/core/event_manager.ts

@@ -59,15 +67,14 @@ export class EventManager implements SyncEngineListener {
    this.syncEngine.subscribe(this);
  }

-  listen(listener: QueryListener): Promise<TargetId> {
+  async listen(listener: QueryListener): Promise<void> {


If I reject the Promise here, then FirestoreClient has to handle the rejected Promise. This is actually pretty nice and keeps the code in EventManager much cleaner. The calling code is also still clean, since it was simple to begin with.

The problem I see is that this makes this code non-testable. The spec tests don't use FirestoreClient, and the integration cannot inject failures. I have made the change regardless and added some special handling in SyncEngine. I am not sure if this is an improvement though.

schmidt-sebastian · 2020-04-23T04:09:49Z

packages/firestore/src/core/event_manager.ts

+        queryInfo.targetId = targetId;
+        queryInfo.viewSnap = snapshot;
+      } catch (e) {
+        if (e.name === 'IndexedDbTransactionError') {


I did end up updating this, but as stated this pushes the error handling to both FirestoreClient and SpecTestRunner.

schmidt-sebastian · 2020-04-23T04:23:03Z

packages/firestore/src/core/sync_engine.ts

@@ -98,6 +98,12 @@ class QueryView {
  ) {}
 }

+/** Result type returned during initial query registration. */
+export interface QueryRegistration {


The issue here is actually the success case - the problem here is the ViewSnapshot for the initial listen needs to be raised. The current flow is:

EventManager's listen() registers target in local data structure

EventManager calls SyncEngine listen()

SyncEngine's listen() calls back EventManager

EventManager looks in local data structure to raise snapshot

SyncEngine finishes listen()

EventManager finishes listen()

This code seems fragile to me The callback in SyncEngine relies on an implementation detail of EventManager. I was very surprised that re-arranging the internal code of EventManager broke tests that were unrelated to my change. Because of this, I think it is better to return a ViewSnapshot.

Having said that, I was able to remove this type after all because EventManager doesn't actually use the target ID (other than to pass it through). I updated listen() to just return the initial snapshot.

schmidt-sebastian · 2020-04-23T04:31:01Z

packages/firestore/src/core/sync_engine.ts

@@ -246,8 +252,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      }
    }

-    this.syncEngineListener!.onWatchChange([viewSnapshot]);


I am not sure I fully understand this, but I hope that my previous comment reliefs your concerns. Let me know if it doesn't.

schmidt-sebastian · 2020-04-28T16:38:48Z

Friendly bump :)

wilhuff · 2020-04-28T17:12:21Z

packages/firestore/src/core/firestore_client.ts

+          listener.onError(
+            new FirestoreError(
+              Code.UNAVAILABLE,
+              `Failed to register query: ${e}`


A few thoughts on this:

From a user's point of view they listen to queries (or get them) but we don't have a public concept of "registration". I think it's probably fine to just say "failed to listen to query", "failed to start a listener to query" or maybe something more generic like "failed to start query".

We should include details of the query that failed (a toString or similar). Whenever people are filing issues about broken queries our first instructions are to have people go figure out which query is causing the problem and they're usually at a loss. We should just include that in the error message.

Updated to use "initialize" and also included the Query in the message.

wilhuff · 2020-04-28T17:15:58Z

packages/firestore/src/core/event_manager.ts

+      queryInfo.viewSnap = await this.syncEngine.listen(query);
+    }
+
+    this.queries.set(query, queryInfo!);


I'm surprised the ! is required here. Isn't queryInfo guaranteed to be set here? (What confuses me is you're not null-checking the assignment to e.g. queryInfo.viewSnap).

It's indeed not needed. It might have been needed before the refactor, but now TS can determine that it is set.

wilhuff · 2020-04-28T17:17:36Z

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

+      .expectEvents(query1, {})
+      .failDatabase()
+      .userListens(query2)
+      .expectEvents(query2, { errorCode: Code.UNKNOWN })


What's the source of the UNKNOWN here? The spec test runner seems to be doing new FirestoreError(Code.UNAVAILABLE, e.message). Is anything even handling errorCode?

We don't validate the error code at all. I added this validation.

wilhuff · 2020-04-28T17:49:34Z

packages/firestore/src/core/event_manager.ts

    }
+
+    if (firstListen) {
+      queryInfo.viewSnap = await this.syncEngine.listen(query);


Nothing seems to populate queryInfo.targetId anymore?

queryInfo.targetId is never read and I removed it.

wilhuff · 2020-04-28T17:51:19Z

packages/firestore/src/core/event_manager.ts

@@ -59,15 +67,14 @@ export class EventManager implements SyncEngineListener {
    this.syncEngine.subscribe(this);
  }

-  listen(listener: QueryListener): Promise<TargetId> {
+  async listen(listener: QueryListener): Promise<void> {


I see what you mean; the spec test has to emulate this for itself. That's not completely awful but you're probably right that the other way was better because it was testable.

Would it help to put the error handling in SyncEngine?

wilhuff

LGTM

schmidt-sebastian force-pushed the mrschmidt/initialwatch branch from 1a67dfc to 56ada10 Compare April 20, 2020 16:19

schmidt-sebastian requested review from wilhuff and removed request for wilhuff April 20, 2020 16:22

schmidt-sebastian force-pushed the mrschmidt/initialwatch branch from 56ada10 to 83f0a22 Compare April 20, 2020 17:23

schmidt-sebastian commented Apr 20, 2020

View reviewed changes

schmidt-sebastian force-pushed the mrschmidt/limitretries branch from 374948d to d2ac3b0 Compare April 20, 2020 20:31

schmidt-sebastian force-pushed the mrschmidt/initialwatch branch from 83f0a22 to a72091c Compare April 21, 2020 01:41

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners April 21, 2020 01:41

schmidt-sebastian changed the base branch from mrschmidt/limitretries to master April 21, 2020 01:41

Don't crash if Target cannot be persisted

45d44ff

schmidt-sebastian force-pushed the mrschmidt/initialwatch branch from a72091c to 45d44ff Compare April 21, 2020 01:45

schmidt-sebastian assigned wilhuff Apr 21, 2020

schmidt-sebastian requested a review from wilhuff April 21, 2020 01:47

schmidt-sebastian added the api: firestore label Apr 21, 2020

wilhuff reviewed Apr 22, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 22, 2020

Review

f28e510

schmidt-sebastian commented Apr 23, 2020

View reviewed changes

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 23, 2020

wilhuff reviewed Apr 28, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 28, 2020

Review

d51c676

schmidt-sebastian removed their assignment Apr 29, 2020

schmidt-sebastian assigned wilhuff Apr 29, 2020

schmidt-sebastian mentioned this pull request Apr 30, 2020

Recovering from IndexedDBTransactionError #2997

Closed

Merge

8a22dbf

wilhuff approved these changes May 1, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff May 1, 2020

schmidt-sebastian merged commit 174521a into master May 1, 2020

scottcrossen pushed a commit that referenced this pull request May 4, 2020

Don't crash if Target cannot be persisted (#2944)

a2cff78

schmidt-sebastian mentioned this pull request May 7, 2020

Offline Mode Improvements #2755

Closed

firebase locked and limited conversation to collaborators Jun 1, 2020

schmidt-sebastian deleted the mrschmidt/initialwatch branch November 9, 2020 22:37

		@@ -1,4 +1,6 @@
		# Unreleased
		- [fixed] Firestore now rejects `onSnapshot()` listeners if they cannot be

Don't crash if Target cannot be persisted #2944

Don't crash if Target cannot be persisted #2944

Uh oh!

Conversation

schmidt-sebastian commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/memory

firebase

Test Logs

Uh oh!

schmidt-sebastian Apr 20, 2020 • 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!

schmidt-sebastian Apr 20, 2020 • 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!

schmidt-sebastian Apr 23, 2020 • 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 • 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!

schmidt-sebastian Apr 23, 2020 • 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 commented Apr 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Apr 20, 2020 •

edited

Loading

google-oss-bot commented Apr 20, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`

schmidt-sebastian Apr 20, 2020 •

edited

Loading

schmidt-sebastian Apr 20, 2020 •

edited

Loading

schmidt-sebastian Apr 23, 2020 •

edited

Loading

schmidt-sebastian left a comment •

edited

Loading

schmidt-sebastian Apr 23, 2020 •

edited

Loading