Defer check whether auth is available #4845

schmidt-sebastian · 2021-04-28T17:17:22Z

Adds support for calling getFirestore before calling getAuth.

Tested manually (our integration tests do not use auth).

changeset-bot · 2021-04-28T17:17:24Z

⚠️ No Changeset found

Latest commit: be1fa3c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2021-04-28T18:23:43Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`c34ac7a`)	Head (`eee135e`)	Diff
browser	283 kB	284 kB	+604 B (+0.2%)
esm2017	226 kB	226 kB	+209 B (+0.1%)
main	530 kB	531 kB	+955 B (+0.2%)
module	283 kB	284 kB	+604 B (+0.2%)
react-native	226 kB	226 kB	+209 B (+0.1%)

`@firebase/firestore-exp`

Type	Base (`c34ac7a`)	Head (`eee135e`)	Diff
browser	223 kB	224 kB	+209 B (+0.1%)
main	506 kB	507 kB	+955 B (+0.2%)
module	223 kB	224 kB	+209 B (+0.1%)
react-native	224 kB	224 kB	+209 B (+0.1%)

`@firebase/firestore-lite`

Type	Base (`c34ac7a`)	Head (`eee135e`)	Diff
browser	72.0 kB	72.4 kB	+444 B (+0.6%)
main	146 kB	147 kB	+1.10 kB (+0.8%)
module	72.0 kB	72.4 kB	+444 B (+0.6%)
react-native	72.2 kB	72.6 kB	+459 B (+0.6%)

`@firebase/firestore/bundle`

Type	Base (`c34ac7a`)	Head (`eee135e`)	Diff
browser	289 kB	290 kB	+604 B (+0.2%)
esm2017	176 kB	176 kB	+209 B (+0.1%)
main	526 kB	527 kB	+955 B (+0.2%)
module	289 kB	290 kB	+604 B (+0.2%)
react-native	176 kB	176 kB	+209 B (+0.1%)

`@firebase/firestore/memory`

Type	Base (`c34ac7a`)	Head (`eee135e`)	Diff
browser	215 kB	216 kB	+604 B (+0.3%)
esm2017	172 kB	172 kB	+209 B (+0.1%)
main	324 kB	325 kB	+479 B (+0.1%)
module	215 kB	216 kB	+604 B (+0.3%)
react-native	172 kB	172 kB	+209 B (+0.1%)

`@firebase/firestore/memory-bundle`

Type	Base (`c34ac7a`)	Head (`eee135e`)	Diff
browser	224 kB	224 kB	+604 B (+0.3%)
esm2017	176 kB	176 kB	+209 B (+0.1%)
main	321 kB	322 kB	+479 B (+0.1%)
module	224 kB	224 kB	+604 B (+0.3%)
react-native	176 kB	176 kB	+209 B (+0.1%)

`firebase`

Type	Base (`c34ac7a`)	Head (`eee135e`)	Diff
firebase-firestore.js	332 kB	332 kB	+528 B (+0.2%)
firebase-firestore.memory.js	266 kB	267 kB	+528 B (+0.2%)
firebase.js	875 kB	875 kB	+532 B (+0.1%)

Test Logs

Base (c34ac7a9): https://github.com/firebase/firebase-js-sdk/actions/runs/811754688
Head (eee135e6): https://github.com/firebase/firebase-js-sdk/actions/runs/811932244

google-oss-bot · 2021-04-28T18:31:17Z

Size Analysis Report

Affected Products

No changes between base commit (c34ac7a) and head commit (f1d081d).

Test Logs

Feiyang1

I got an internal assertion failure unexpected state if auth is never initialized, or initialized too late. And requests seem to be queued indefinitely until Auth is initialized.

schmidt-sebastian · 2021-04-29T14:42:29Z

The assertion should be gone as of 1e75d78

Feiyang1 · 2021-04-29T21:31:33Z

Thanks. The error went away. Now I found a use case where Firestore still doesn't wait for auth initialization:

const firestore = getFirestore(firebaseApp);
setLogLevel("debug");

// this getDoc() is important for repro. If we don't make a getDoc here, the getDoc in setTimeout will succeed.
getDoc(doc(firestore, 'coll/New York')).then(v => {
    console.log('sync firestore get', v.data());
});


setTimeout(() => {
    // assume there is a valid user
    getAuth(); 
    
    // getDoc fails with permission denied, but it should succeed since it's called after getAuth().
    getDoc(doc(firestore, 'coll/New York')).then(v => {
        console.log('Async firestore get', v.data());
    });
}, 500);

For repro, you can clone https://github.com/Feiyang1/playground/tree/fs-auth-init and use the project in the rollup directory.

You mentioned we may need to queue up work that uses the user, or even the auth, is that what we need to do to make it work?

schmidt-sebastian · 2021-04-29T21:41:02Z

I think this will be solved by your "callback" PR that replaces the Promise in the initialization. Do you still have a link to that? I can apply this here or we can merge that PR before we test here further.

Feiyang1 · 2021-04-29T21:58:36Z

Yes! https://github.com/firebase/firebase-js-sdk/commits/fei-firestore-wait

You may need to resolve some merge conflicts. Thanks!

Feiyang1 · 2021-04-30T03:04:09Z

I took the liberty to merge the component change into your PR, but It still doesn't work.

schmidt-sebastian · 2021-04-30T15:18:33Z

I took the liberty to merge the component change into your PR, but It still doesn't work.

I will take over from here :)

schmidt-sebastian · 2021-04-30T16:49:33Z

Updated. Tested with no getDoc initially, with getDoc initially and with no getAuth.

schmidt-sebastian · 2021-05-04T16:21:35Z

This is now completely rewritten. The SDK now blocks on the availability of the use in the Credential Provider itself, which allows the SDK to block twice: Once when we check whether Auth is available at startup time and then once again if Auth is loaded later. We also synchronously invoke the user change handler (right as we release the block), which ensures that the user change gets executed before everything else.

Feiyang1

I think I understand most parts, but left a couple of questions.

I think we also need to do something similar in Database.

packages/firestore/src/api/credentials.ts

Feiyang1 · 2021-05-04T19:22:33Z

packages/firestore/src/api/credentials.ts

+        // raised. The initial event itself is invoked synchronously in
+        // `awaitTokenAndRaiseInitialEvent()`.
+        this.asyncQueue.enqueueRetryable(() => {
+          this.changeListener(this.currentUser);


In the registerAuth(), we do

this.awaitTokenAndRaiseInitialEvent(); this.auth.addAuthTokenListener(this.tokenListener);

We already invoke this.changeListener in the asyncQueue in awaitTokenAndRaiseInitialEvent, should we not do it again in this.tokenListener?

I think I addressed this. I am not able to verify this right now, but will do a test run later today. BTW, this was meant to be addressed by the buggy code around initialEventRaised

Feiyang1 · 2021-05-04T19:31:55Z

packages/firestore/src/api/credentials.ts

+   * Whether the initial token event has been raised. This can go back to
+   * `false` if Firestore first starts without Auth and Auth is loaded later.
+   */
+  private initialEventRaised: boolean = false;


This variable doesn't seem to do anything, because it's only set in awaitTokenAndRaiseInitialEvent which is synchronous. It's not used in awaitTokenAndRaiseInitialEvent() itself and is always set to true at the end of the function, so it seems the outside world will always see this variable as true.

Hm, this bothers me. It is meant to prevent double-firing of the change listener. I tested that it doesn't fire twice manually, but what you say is also true - this variable and its check do not do anything. I have fixed the variable and am now resetting it on the queue rather than synchronously.

Co-authored-by: Feiyang <[email protected]>

Feiyang1 · 2021-05-04T23:01:11Z

packages/firestore/src/api/credentials.ts

+      // Create a new deferred Promise that gets resolved when we receive the
+      // next token. Ensure that all previous Promises also get resolved.
+      const awaitToken = new Deferred<void>();
+      void awaitToken.promise.then(() => awaitToken.resolve());


then only fires if awaitToken resolves, why do we resolve it again in then? The comment suggests it ensures all previous Promises also get resolved? But I don't get how that works.

This was meant to resolve the existing this.receivedUser Promise if another one was created later on. I changed how the listener gets called if auth is not used, and am no longer using the Promise twice. This allowed me to remove this logic.

Feiyang1 · 2021-05-04T23:06:35Z

packages/firestore/src/api/credentials.ts

+   * `awaitTokenAndRaiseInitialEvent()` is also used to block Firestore until
+   * Auth is fully initialized.
+   *
+   * This function also invokes the change listener synchronously once a token


Do you mean asynchronously, since it happens in the asyncQueue?

I meant "synchronously with the code that resolves the token". I have reworded it.

Feiyang1 · 2021-05-04T23:17:57Z

packages/firestore/src/api/credentials.ts

+      }
+    }, 0);
+
+    this.awaitTokenAndRaiseInitialEvent();


Is this.asyncQueue set at this point?

My understanding is it is not, so awaitTokenAndRaiseInitialEvent doesn't do anything except for setting this.invokeChangeListener = false; which will lead to wrong behavior if auth is not available - the following code in this.tokenListener` will not execute:

if (this.invokeChangeListener) { this.asyncQueue!.enqueueRetryable(() => this.changeListener(this.currentUser) ); }

This seems to work in code though. Firestore will not receive a null user when auth is not initialized, but nothing seems to break.

I changed the logic for invokeChangeListener to only set it to true if the AsyncQueue is available. This should address this concern.

…-js-sdk into mrschmidt/optionalauth

schmidt-sebastian · 2021-05-05T01:37:36Z

This should be ready for review again.

Feiyang1 · 2021-05-05T05:27:34Z

packages/firestore/src/api/credentials.ts

+        if (auth) {
+          registerAuth(auth);
+        } else if (this.invokeChangeListener) {
+          // If auth is still not available, invoke tokenListener once with null


Comment needs update.

Feiyang1 · 2021-05-05T05:50:01Z

Tested the following manually, and they all worked. Yay!

initializeAuth before getFirestore
initializeAuth after getFirestore synchronously
initializeAuth after getFirestore asynchronously
initializeAuth never called

schmidt-sebastian force-pushed the mrschmidt/optionalauth branch from 8d2b847 to 07ccc28 Compare April 28, 2021 17:19

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners April 28, 2021 17:59

schmidt-sebastian assigned Feiyang1 Apr 28, 2021

schmidt-sebastian added the api: firestore label Apr 28, 2021

schmidt-sebastian changed the title ~~Defer check whether auth is avaiable~~ Defer check whether auth is available Apr 29, 2021

Feiyang1 reviewed Apr 29, 2021

View reviewed changes

schmidt-sebastian force-pushed the mrschmidt/optionalauth branch from 44f5c2f to 6e9133e Compare May 4, 2021 16:19

schmidt-sebastian requested a review from jsdt as a code owner May 4, 2021 16:19

schmidt-sebastian force-pushed the mrschmidt/optionalauth branch 3 times, most recently from 3e3015e to f0355ed Compare May 4, 2021 16:37

Support lazy-loaded Auth

99b79f6

schmidt-sebastian force-pushed the mrschmidt/optionalauth branch from f0355ed to 99b79f6 Compare May 4, 2021 16:37

Feiyang1 reviewed May 4, 2021

View reviewed changes

schmidt-sebastian and others added 3 commits May 4, 2021 14:14

Update packages/firestore/src/api/credentials.ts

1ff8800

Co-authored-by: Feiyang <[email protected]>

Update packages/firestore/src/api/credentials.ts

c15ae9f

Co-authored-by: Feiyang <[email protected]>

Review

41b5dc5

Feiyang1 reviewed May 4, 2021

View reviewed changes

schmidt-sebastian added 6 commits May 4, 2021 18:15

Simplify

51718f1

Merge branch 'master' into mrschmidt/optionalauth

55590e6

Update provider.ts

3624380

Cleanup

0faa3d9

Comment cleanup

958bcdf

Merge branch 'mrschmidt/optionalauth' of github.com:firebase/firebase…

bc06883

…-js-sdk into mrschmidt/optionalauth

Feiyang1 approved these changes May 5, 2021

View reviewed changes

Review

be1fa3c

schmidt-sebastian merged commit a13f5a0 into master May 5, 2021

schmidt-sebastian deleted the mrschmidt/optionalauth branch May 5, 2021 17:05

firebase locked and limited conversation to collaborators Jun 5, 2021

Defer check whether auth is available #4845

Defer check whether auth is available #4845

Conversation

schmidt-sebastian commented Apr 28, 2021 • edited Loading

changeset-bot bot commented Apr 28, 2021 • edited Loading

⚠️ No Changeset found

google-oss-bot commented Apr 28, 2021 • edited Loading

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore-exp

@firebase/firestore-lite

@firebase/firestore/bundle

@firebase/firestore/memory

@firebase/firestore/memory-bundle

firebase

Test Logs

google-oss-bot commented Apr 28, 2021 • edited Loading

Size Analysis Report

Affected Products

Test Logs

Feiyang1 left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented Apr 29, 2021

Feiyang1 commented Apr 29, 2021

schmidt-sebastian commented Apr 29, 2021

Feiyang1 commented Apr 29, 2021

Feiyang1 commented Apr 30, 2021

schmidt-sebastian commented Apr 30, 2021

schmidt-sebastian commented Apr 30, 2021

schmidt-sebastian commented May 4, 2021

Feiyang1 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

Feiyang1 May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feiyang1 commented May 5, 2021

schmidt-sebastian commented Apr 28, 2021 •

edited

Loading

changeset-bot bot commented Apr 28, 2021 •

edited

Loading

google-oss-bot commented Apr 28, 2021 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore-exp`

`@firebase/firestore-lite`

`@firebase/firestore/bundle`

`@firebase/firestore/memory`

`@firebase/firestore/memory-bundle`

`firebase`

google-oss-bot commented Apr 28, 2021 •

edited

Loading

Feiyang1 May 5, 2021 •

edited

Loading