Make notifyLocalViewChanges idempotent #3044

schmidt-sebastian · 2020-05-10T02:40:04Z

This changes notifyLocalViewChanges so that it has not side effects if the transaction fails. The main change is that I removed the indirection of the reference delegate to ask the LocalStore what documents are "pinned". Instead, the Local Store directly tells the reference delegate about every document that is added to a View.

Adresses #2755

google-oss-bot · 2020-05-10T02:47:33Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (a57dac5) Head (c7d168b) Diff

browser 249 kB 249 kB -152 B (-0.1%)

esm2017 194 kB 194 kB -148 B (-0.1%)

main 490 kB 490 kB -452 B (-0.1%)

module 247 kB 247 kB -152 B (-0.1%)
@firebase/firestore/memory

Type Base (a57dac5) Head (c7d168b) Diff

browser 190 kB 190 kB -80 B (-0.0%)

esm2017 148 kB 148 kB -98 B (-0.1%)

main 367 kB 366 kB -277 B (-0.1%)

module 188 kB 188 kB -80 B (-0.0%)

`firebase`

Type	Base (`a57dac5`)	Head (`c7d168b`)	Diff
firebase-firestore.js	288 kB	288 kB	-158 B (-0.1%)
firebase-firestore.memory.js	230 kB	230 kB	-83 B (-0.0%)
firebase.js	821 kB	821 kB	-158 B (-0.0%)

Test Logs

Base (a57dac5a): https://github.com/firebase/firebase-js-sdk/actions/runs/98717144
Head (c7d168b2): https://github.com/firebase/firebase-js-sdk/actions/runs/102136500

schmidt-sebastian · 2020-05-11T03:08:35Z

packages/firestore/src/core/sync_engine.ts

@@ -682,8 +682,7 @@ export class SyncEngine implements RemoteSyncer {
    this.queriesByTarget.delete(targetId);

    if (this.isPrimaryClient) {
-      const limboKeys = this.limboDocumentRefs.referencesForId(targetId);
-      this.limboDocumentRefs.removeReferencesForId(targetId);
+      const limboKeys = this.limboDocumentRefs.removeReferencesForId(targetId);


Combined these lines since removeReferencesForId already returns the references for the target.

schmidt-sebastian · 2020-05-11T03:10:31Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -1141,7 +1137,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
      .removeTargets(txn, upperBound, activeTargetIds);
  }

-  removeMutationReference(
+  markPotentiallyOrphaned(


Renamed this method as I need to use it to trigger key to be considered potentially orphaned. Only then with Eager GC pick it up during its GC run. Other names welcome, including removeMutationReference, which reveals less implementation details.

schmidt-sebastian · 2020-05-11T03:10:59Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -1070,8 +1070,6 @@ function clientMetadataStore(

 /** Provides LRU functionality for IndexedDB persistence. */
 export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
-  private inMemoryPins: ReferenceSet | null = null;


I removed the inMemoryPins redirection to the LocalStore. This was possible because LocalStore now directly pins all documents in a View via addReference.

schmidt-sebastian · 2020-05-11T03:12:53Z

packages/firestore/src/local/local_store.ts

-        targetId
-      );
-
-      if (!viewChange.fromCache) {


Note that this part of the loop is just moved to below the transaction. The goal is that if the transaction fails, this code doesn't get executed.

schmidt-sebastian · 2020-05-11T03:13:15Z

packages/firestore/src/local/local_store.ts

              (key: DocumentKey) =>
-                this.persistence.referenceDelegate.removeReference(txn, key)
+                this.persistence.referenceDelegate.addReference(


This is the call that makes it possible to remove the local pins.

schmidt-sebastian · 2020-05-11T03:14:07Z

packages/firestore/src/local/local_store.ts

-          ).next(() => {
-            this.persistence.referenceDelegate.removeTarget(txn, targetData!);
-          });
+          return this.persistence.referenceDelegate.removeTarget(


The reference delegate now knows internally what documents are orphaned in the view.

schmidt-sebastian · 2020-05-11T03:15:42Z

packages/firestore/src/local/memory_target_cache.ts

-    const promises: Array<PersistencePromise<void>> = [];
-    if (referenceDelegate) {
-      keys.forEach(key => {
-        promises.push(referenceDelegate.addReference(txn, key));


We could keep this call... but all this does now is mark a document as non-orphaned if it was previously orphaned. The keys now also added to the target as part of notifyLocalViewChanges .

var-const · 2020-05-12T02:13:54Z

packages/firestore/src/local/memory_persistence.ts

-  private inMemoryPins: ReferenceSet | null = null;
+  /** Manages all documents that are active in Query views. */
+  private localViewReferences: ReferenceSet = new ReferenceSet();
+  /** The list of documents that are potentially GCed after each transaction. */
  private _orphanedDocuments: Set<DocumentKey> | null = null;


Question: why is this variable prefixed with an underscore, while localViewReferences isn't?

In general, we don't use underscore prefixes for classes that are not part of the private API. This is an exception as orphanedDocuments has a getter that verifies that is is set correctly. orphanedDocuments is cleared between each transaction run.

var-const · 2020-05-12T02:15:31Z

packages/firestore/src/local/memory_persistence.ts

@@ -184,7 +184,9 @@ export interface MemoryReferenceDelegate extends ReferenceDelegate {
 }

 export class MemoryEagerDelegate implements MemoryReferenceDelegate {
-  private inMemoryPins: ReferenceSet | null = null;
+  /** Manages all documents that are active in Query views. */


Nit: I always find that manage is one of the words that can usually be replaced with something more specific. Even keeps track of, to me, seems more specific.

I hope this is not a general stance on management :) Updated to use "track".

Make notifyLocalViewChanges idempotent

6b3fc3f

Lint

98dc00d

schmidt-sebastian commented May 11, 2020

View reviewed changes

schmidt-sebastian requested a review from var-const May 11, 2020 03:19

schmidt-sebastian assigned var-const May 11, 2020

schmidt-sebastian added the api: firestore label May 11, 2020

var-const approved these changes May 12, 2020

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const May 12, 2020

Tracks

a06fc87

schmidt-sebastian merged commit 74621b7 into master May 12, 2020

firebase locked and limited conversation to collaborators Jun 12, 2020

schmidt-sebastian deleted the mrschmidt/idempotentrefsv2 branch November 9, 2020 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make notifyLocalViewChanges idempotent #3044

Make notifyLocalViewChanges idempotent #3044

Uh oh!

schmidt-sebastian commented May 10, 2020 •

edited

Loading

Uh oh!

google-oss-bot commented May 10, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`

Uh oh!

schmidt-sebastian May 11, 2020

Uh oh!

schmidt-sebastian May 11, 2020

Uh oh!

schmidt-sebastian May 11, 2020

Uh oh!

schmidt-sebastian May 11, 2020

Uh oh!

schmidt-sebastian May 11, 2020

Uh oh!

schmidt-sebastian May 11, 2020

Uh oh!

schmidt-sebastian May 11, 2020

Uh oh!

var-const May 12, 2020

Uh oh!

schmidt-sebastian May 12, 2020

Uh oh!

var-const May 12, 2020

Uh oh!

schmidt-sebastian May 12, 2020

Uh oh!

Uh oh!

Make notifyLocalViewChanges idempotent #3044

Make notifyLocalViewChanges idempotent #3044

Uh oh!

Conversation

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

Uh oh!

Uh oh!

google-oss-bot commented May 10, 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!

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!

Uh oh!

schmidt-sebastian commented May 10, 2020 •

edited

Loading

google-oss-bot commented May 10, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`