Skip to content

Add experimentalForceOwningTab for Web Worker support #2197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 33 commits into from
Jun 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
f0287d4
cleaned up
Sep 26, 2019
15cf32e
fix tests
Sep 27, 2019
66655d0
remove comments
Sep 27, 2019
9ac6e2d
fix michael comments
Sep 30, 2019
d813905
Merge branch 'master' into bc/ww-persist
Oct 21, 2019
347b722
Merge branch 'master' into bc/ww-persist
Nov 12, 2019
91404e3
[AUTOMATED]: Prettier Code Styling
Nov 12, 2019
00c6dc7
Merge branch 'release' into bc/ww-persist
Nov 14, 2019
6dc5e51
Merge branch 'master' into bc/ww-persist
Dec 10, 2019
fef8971
Merge branch 'master' into bc/ww-persist
Feb 13, 2020
8366646
Merge branch 'master' into bc/ww-persist
May 7, 2020
741e88e
update post API council
May 7, 2020
86ca4aa
[AUTOMATED]: Prettier Code Styling
May 7, 2020
16479d4
[AUTOMATED]: License Headers
May 7, 2020
0784827
Merge branch 'master' into bc/ww-persist
May 7, 2020
59d07ee
update error message for primary lease unavailability
May 8, 2020
5dae5b6
Merge branch 'master' into bc/ww-persist
May 13, 2020
71785f2
add changelog
May 13, 2020
ca02909
Merge branch 'master' into bc/ww-persist
May 13, 2020
c5f491a
Merge branch 'master' into bc/ww-persist
May 13, 2020
08f8fb9
resolve comments
May 13, 2020
398c330
Merge branch 'master' into bc/ww-persist
May 18, 2020
6b6db57
Merge branch 'master' into bc/ww-persist
May 18, 2020
b178a75
resolve comments and post-merge issues
May 18, 2020
7d8e41a
update isAvailable() logic + comments round3
May 20, 2020
6d20b20
add forceOwningTab logic back to canActAsPrimary, remove from updateC…
May 21, 2020
016aed8
update forceOwningTab comment
May 27, 2020
31a8772
move forceOwningTab logic up in canActAsPrimary
May 27, 2020
c7cbf9a
Merge branch 'master' into bc/ww-persist
May 28, 2020
be3a0d4
update new test from master
May 28, 2020
47cd1f6
Merge branch 'master' into bc/ww-persist
May 28, 2020
0f45287
s/'true'/
Jun 1, 2020
9cfe34d
remove project.json
Jun 1, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions packages/firebase/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7760,7 +7760,7 @@ declare namespace firebase.firestore {
export interface PersistenceSettings {
/**
* Whether to synchronize the in-memory state of multiple tabs. Setting this
* to 'true' in all open tabs enables shared access to local persistence,
* to `true` in all open tabs enables shared access to local persistence,
* shared execution of queries and latency-compensated local document updates
* across all connected instances.
*
Expand All @@ -7772,14 +7772,27 @@ declare namespace firebase.firestore {

/**
* Whether to synchronize the in-memory state of multiple tabs. Setting this
* to 'true' in all open tabs enables shared access to local persistence,
* to `true` in all open tabs enables shared access to local persistence,
* shared execution of queries and latency-compensated local document updates
* across all connected instances.
*
* @deprecated This setting is deprecated. To enabled synchronization between
* @deprecated This setting is deprecated. To enable synchronization between
* multiple tabs, please use `synchronizeTabs: true` instead.
*/
experimentalTabSynchronization?: boolean;

/**
* Whether to force enable persistence for the client. This cannot be used
* with `synchronizeTabs:true` and is primarily intended for use with Web
* Workers. Setting this to `true` will enable persistence, but cause other
* tabs using persistence to fail.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: If/when we ship this for real we should add more detail about exactly how other tabs will fail (so that developers can potentially write code to detect it, etc.).

*
* This setting may be removed in a future release. If you find yourself
* using it for a specific use case or run into any issues, please tell us
* about it in
* https://github.com/firebase/firebase-js-sdk/issues/983.
*/
experimentalForceOwningTab?: boolean;
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to jot this down so I can point to it in 20 years: This name is so amazingly non-obvious that it really bothers me. It bothers me even more that there was no feedback on this from the API council. I don't understand how that this got approved without a single comment from Michael Bleigh, who has been the one true shepherd of the Firestore Web API. It is also still technically missing one more LGTM, as the Games TL is not yet a voting council member.

Since I don't want to just rant without providing an alternative suggestion, here you go:

experimentalForceExclusivePersistenceAccess
experimentalForcePersistenceOwnership

I also think that this could work with synchronizeTabs if it is used outside of WebWorkers. If this is not a pattern we want to support, then maybe we should have WebWorkers in the name?

experimentalWebWorkerPersistence

We don't necessarily need to open this up for discussion again, but I can almost guarantee that if you randomly surveyed people outside of our team, no one would be able to say what experimentalForceOwningTab does and why they would need to turn it on to use Firestore with WebWorkers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think that this could work with synchronizeTabs if it is used outside of WebWorkers. If this is not a pattern we want to support, then maybe we should have WebWorkers in the name?

What is the use case for this feature with synchronizeTabs? Limiting the name to web workers only would be misleading, since this should work for Web, Shared, and Service Workers as well as other non-worker environments that don't have LocalStorage availability.

Not sure what proper next steps are here. I can definitely see your perspective on how forceOwningTabs would be confusing, but I'm not sure how to reduce confusion without introducing the concept of persistence leases, which was a decision factor during the API discussion. One option I see is to release this as experimentalForceOwningTab for now and then rename it to something else if/when we make remove the experimental flag on this based on developer feedback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use case might be to pin an owner tab. Not sure why that might be useful, but it would avoid primary to secondary tab transition which AFAIK haven't been a problem.

As for the name:

To me, WebWorker is the umbrella term, but I might be mistaken (and propose experimentalBackgroundWorkerPersistence instead).

My main problem with the API name is that experimentalForceOwningTab only says what it does but doesn't say why it does it or what feature it unblocks. The fact that it enables WebWorker/SharedWorker/ServiceWorker persistence is not obvious from the name, and it isn't even document in the API description. The only place where it shows up is in the title of the issue that is linked. A user that wants to use Firestore in a WebWorker has no obvious way to discover that this is the setting they need to enable.

If you are happy with the name, feel free to ignore. This has been discussed for quite some time.

}

export type LogLevel = 'debug' | 'error' | 'silent';
Expand Down
1 change: 1 addition & 0 deletions packages/firestore-types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export interface Settings {
export interface PersistenceSettings {
synchronizeTabs?: boolean;
experimentalTabSynchronization?: boolean;
experimentalForceOwningTab?: boolean;
}

export type LogLevel = 'debug' | 'error' | 'silent';
Expand Down
3 changes: 3 additions & 0 deletions packages/firestore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
in IndexedDB. Previously, these errors crashed the client.
- [fixed] Fixed a source of IndexedDB-related crashes for tabs that receive
multi-tab notifications while the file system is locked.
- [feature] Added an `experimentalForceOwningTab` setting that can be used to
enable persistence in environments without LocalStorage, which allows
persistence to be used in Web Workers (#983).

# 1.10.2
- [fixed] Temporarily reverted the use of window.crypto to generate document
Expand Down
15 changes: 14 additions & 1 deletion packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
}

let synchronizeTabs = false;
let experimentalForceOwningTab = false;

if (settings) {
if (settings.experimentalTabSynchronization !== undefined) {
Expand All @@ -395,12 +396,24 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
settings.synchronizeTabs ??
settings.experimentalTabSynchronization ??
DEFAULT_SYNCHRONIZE_TABS;

experimentalForceOwningTab = settings.experimentalForceOwningTab
? settings.experimentalForceOwningTab
: false;

if (synchronizeTabs && experimentalForceOwningTab) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
"The 'experimentalForceOwningTab' setting cannot be used with 'synchronizeTabs'."
);
}
}

return this.configureClient(this._componentProvider, {
durable: true,
cacheSizeBytes: this._settings.cacheSizeBytes,
synchronizeTabs
synchronizeTabs,
forceOwningTab: experimentalForceOwningTab
});
}

Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/src/core/component_provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,8 @@ export class IndexedDbComponentProvider extends MemoryComponentProvider {
LruParams.withCacheSize(cfg.persistenceSettings.cacheSizeBytes),
cfg.asyncQueue,
serializer,
this.sharedClientState
this.sharedClientState,
cfg.persistenceSettings.forceOwningTab
);
}

Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/core/firestore_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export type PersistenceSettings =
readonly durable: true;
readonly cacheSizeBytes: number;
readonly synchronizeTabs: boolean;
readonly forceOwningTab: boolean;
};

/**
Expand Down
84 changes: 60 additions & 24 deletions packages/firestore/src/local/indexeddb_persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ const CLIENT_METADATA_REFRESH_INTERVAL_MS = 4000;
const PRIMARY_LEASE_EXCLUSIVE_ERROR_MSG =
'Failed to obtain exclusive access to the persistence layer. ' +
'To allow shared access, make sure to invoke ' +
'`enablePersistence()` with `synchronizeTabs:true` in all tabs.';
'`enablePersistence()` with `synchronizeTabs:true` in all tabs. ' +
'If you are using `experimentalForceOwningTab:true`, make sure that only ' +
'one tab has persistence enabled at any given time.';
const UNSUPPORTED_PLATFORM_ERROR_MSG =
'This platform is either missing' +
' IndexedDB or is known to have an incomplete implementation. Offline' +
Expand Down Expand Up @@ -190,7 +192,7 @@ export class IndexedDbPersistence implements Persistence {
static MAIN_DATABASE = 'main';

private readonly document: Document | null;
private readonly window: Window;
private readonly window: Window | null;

// Technically `simpleDb` should be `| undefined` because it is
// initialized asynchronously by start(), but that would be more misleading
Expand Down Expand Up @@ -225,18 +227,29 @@ export class IndexedDbPersistence implements Persistence {
private readonly targetCache: IndexedDbTargetCache;
private readonly indexManager: IndexedDbIndexManager;
private readonly remoteDocumentCache: IndexedDbRemoteDocumentCache;
private readonly webStorage: Storage;
private readonly webStorage: Storage | null;
readonly referenceDelegate: IndexedDbLruDelegate;

constructor(
/**
* Whether to synchronize the in-memory state of multiple tabs and share
* access to local persistence.
*/
private readonly allowTabSynchronization: boolean,

private readonly persistenceKey: string,
private readonly clientId: ClientId,
platform: Platform,
lruParams: LruParams,
private readonly queue: AsyncQueue,
serializer: JsonProtoSerializer,
private readonly sequenceNumberSyncer: SequenceNumberSyncer
private readonly sequenceNumberSyncer: SequenceNumberSyncer,

/**
* If set to true, forcefully obtains database access. Existing tabs will
* no longer be able to access IndexedDB.
*/
private readonly forceOwningTab: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind adding a short inline comment for this and allowTabSynchronization? The rest of the arguments seem obvious.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this would say something about what the consequences are:

"If set to true, forcefully obtains database access. Existing tabs will no longer be able to access IndexedDB."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

) {
if (!IndexedDbPersistence.isAvailable()) {
throw new FirestoreError(
Expand All @@ -258,14 +271,19 @@ export class IndexedDbPersistence implements Persistence {
this.serializer,
this.indexManager
);
this.window = platform.window;
if (platform.window && platform.window.localStorage) {
this.window = platform.window;
this.webStorage = this.window.localStorage;
this.webStorage = platform.window.localStorage;
} else {
throw new FirestoreError(
Code.UNIMPLEMENTED,
'IndexedDB persistence is only available on platforms that support LocalStorage.'
);
this.webStorage = null;
if (forceOwningTab === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-Tab persistence requires LocalStore. I think WebSharedStorageClientState will reject startup without it, but it might be worth getting a second pair of eyes to look at this code and its callers:

if (!WebStorageSharedClientState.isAvailable(this.platform)) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now throw an error if synchronizeTabs is used with experimentalForceOwningTab, and we throw an error if LocalStorage is unavailable in WebStorageSharedClientState. Which case are we not covering?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to throw here as well if LocalStorage wasn't available. It looks like you came to the same conclusion as I did and that we are still throwing, just somewhere else. LGTM.

logError(
LOG_TAG,
'LocalStorage is unavailable. As a result, persistence may not work ' +
'reliably. In particular enablePersistence() could fail immediately ' +
'after refreshing the page.'
);
}
}
}

Expand All @@ -287,7 +305,9 @@ export class IndexedDbPersistence implements Persistence {
this.simpleDb = db;
// NOTE: This is expected to fail sometimes (in the case of another tab already
// having the persistence lock), so it's the first thing we should do.
return this.updateClientMetadataAndTryBecomePrimary();
return this.updateClientMetadataAndTryBecomePrimary(
this.forceOwningTab
);
})
.then(() => {
if (!this.isPrimary && !this.allowTabSynchronization) {
Expand Down Expand Up @@ -384,7 +404,9 @@ export class IndexedDbPersistence implements Persistence {
* primary state listener if the client either newly obtained or released its
* primary lease.
*/
private updateClientMetadataAndTryBecomePrimary(): Promise<void> {
private updateClientMetadataAndTryBecomePrimary(
forceOwningTab = false
): Promise<void> {
return this.runTransaction(
'updateClientMetadataAndTryBecomePrimary',
'readwrite',
Expand Down Expand Up @@ -519,11 +541,13 @@ export class IndexedDbPersistence implements Persistence {
// Ideally we'd delete the IndexedDb and LocalStorage zombie entries for
// the client atomically, but we can't. So we opt to delete the IndexedDb
// entries first to avoid potentially reviving a zombied client.
inactiveClients.forEach(inactiveClient => {
this.window.localStorage.removeItem(
this.zombiedClientLocalStorageKey(inactiveClient.clientId)
);
});
if (this.webStorage) {
for (const inactiveClient of inactiveClients) {
this.webStorage.removeItem(
this.zombiedClientLocalStorageKey(inactiveClient.clientId)
);
}
}
}
}

Expand Down Expand Up @@ -558,6 +582,9 @@ export class IndexedDbPersistence implements Persistence {
private canActAsPrimary(
txn: PersistenceTransaction
): PersistencePromise<boolean> {
if (this.forceOwningTab) {
return PersistencePromise.resolve<boolean>(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably drop the generic since it can be inferred.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why TS can't infer it, but the error I get is:
Type 'PersistencePromise<true>' is not assignable to type 'PersistencePromise<boolean>'.

}
const store = primaryClientStore(txn);
return store
.get(DbPrimaryClient.key)
Expand All @@ -578,6 +605,7 @@ export class IndexedDbPersistence implements Persistence {
// foreground.
// - every clients network is disabled and no other client's tab is in
// the foreground.
// - the `forceOwningTab` setting was passed in.
if (currentLeaseIsValid) {
if (this.isLocalClient(currentPrimary) && this.networkEnabled) {
return true;
Expand Down Expand Up @@ -853,8 +881,9 @@ export class IndexedDbPersistence implements Persistence {

if (currentLeaseIsValid && !this.isLocalClient(currentPrimary)) {
if (
!this.allowTabSynchronization ||
!currentPrimary!.allowTabSynchronization
!this.forceOwningTab &&
(!this.allowTabSynchronization ||
!currentPrimary!.allowTabSynchronization)
) {
throw new FirestoreError(
Code.FAILED_PRECONDITION,
Expand Down Expand Up @@ -983,7 +1012,7 @@ export class IndexedDbPersistence implements Persistence {
* handler.
*/
private attachWindowUnloadHook(): void {
if (typeof this.window.addEventListener === 'function') {
if (typeof this.window?.addEventListener === 'function') {
this.windowUnloadHandler = () => {
// Note: In theory, this should be scheduled on the AsyncQueue since it
// accesses internal state. We execute this code directly during shutdown
Expand All @@ -1003,10 +1032,10 @@ export class IndexedDbPersistence implements Persistence {
private detachWindowUnloadHook(): void {
if (this.windowUnloadHandler) {
debugAssert(
typeof this.window.removeEventListener === 'function',
typeof this.window?.removeEventListener === 'function',
"Expected 'window.removeEventListener' to be a function"
);
this.window.removeEventListener('unload', this.windowUnloadHandler);
this.window!.removeEventListener('unload', this.windowUnloadHandler);
this.windowUnloadHandler = null;
}
}
Expand All @@ -1019,8 +1048,9 @@ export class IndexedDbPersistence implements Persistence {
private isClientZombied(clientId: ClientId): boolean {
try {
const isZombied =
this.webStorage.getItem(this.zombiedClientLocalStorageKey(clientId)) !==
null;
this.webStorage?.getItem(
this.zombiedClientLocalStorageKey(clientId)
) !== null;
logDebug(
LOG_TAG,
`Client '${clientId}' ${
Expand All @@ -1040,6 +1070,9 @@ export class IndexedDbPersistence implements Persistence {
* clients are ignored during primary tab selection.
*/
private markClientZombied(): void {
if (!this.webStorage) {
return;
}
try {
this.webStorage.setItem(
this.zombiedClientLocalStorageKey(this.clientId),
Expand All @@ -1053,6 +1086,9 @@ export class IndexedDbPersistence implements Persistence {

/** Removes the zombied client entry if it exists. */
private removeClientZombiedEntry(): void {
if (!this.webStorage) {
return;
}
try {
this.webStorage.removeItem(
this.zombiedClientLocalStorageKey(this.clientId)
Expand Down
11 changes: 2 additions & 9 deletions packages/firestore/src/local/simple_db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export class SimpleDb {
// suggests IE9 and older WebKit browsers handle upgrade
// differently. They expect setVersion, as described here:
// https://developer.mozilla.org/en-US/docs/Web/API/IDBVersionChangeRequest/setVersion
const request = window.indexedDB.open(name, version);
const request = indexedDB.open(name, version);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised this works with the IndexedDB shim. We only register on window.indexedDB:

get indexedDB(): IDBFactory | null {

The CI is green though.


request.onsuccess = (event: Event) => {
const db = (event.target as IDBOpenDBRequest).result;
Expand Down Expand Up @@ -145,21 +145,14 @@ export class SimpleDb {

/** Returns true if IndexedDB is available in the current environment. */
static isAvailable(): boolean {
if (typeof window === 'undefined' || window.indexedDB == null) {
if (typeof indexedDB === 'undefined') {
return false;
}

if (SimpleDb.isMockPersistence()) {
return true;
}

// In some Node environments, `window` is defined, but `window.navigator` is
// not. We don't support IndexedDB persistence in Node if the
// isMockPersistence() check above returns false.
if (window.navigator === undefined) {
return false;
}

// We extensively use indexed array values and compound keys,
// which IE and Edge do not support. However, they still have indexedDB
// defined on the window, so we need to check for them here and make sure
Expand Down
Loading