Skip to content

Factor out Multi-Tab code #2882

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.

Sign up for GitHub

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 17 commits into from
Apr 21, 2020
Merged

Factor out Multi-Tab code #2882

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
73c5947
Factor out Multi-Tab code
schmidt-sebastian Apr 8, 2020
7178633
Merge
schmidt-sebastian Apr 8, 2020
b7766c0
Merge
schmidt-sebastian Apr 9, 2020
2448b16
Fix integration tests
schmidt-sebastian Apr 10, 2020
d9b0725
Cleanup
schmidt-sebastian Apr 18, 2020
7835818
Refactor applyOnlineStateChange
schmidt-sebastian Apr 20, 2020
4eee9a1
Merge
schmidt-sebastian Apr 20, 2020
dc09fd1
Undo some of the cleanup
schmidt-sebastian Apr 20, 2020
86ee9fa
MockIndexedDbPersistence
schmidt-sebastian Apr 20, 2020
fefc69c
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Apr 20, 2020
30a8251
Review
schmidt-sebastian Apr 20, 2020
114be4b
Merge branch 'master' into mrschmidt/nomultitabv2
schmidt-sebastian Apr 20, 2020
a0bc0ce
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Apr 20, 2020
ba98a20
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Apr 20, 2020
4c3e474
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Apr 20, 2020
7ef2d47
Merge
schmidt-sebastian Apr 21, 2020
c4d6a84
Fix merge
schmidt-sebastian Apr 21, 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
4 changes: 2 additions & 2 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
constructor(
databaseIdOrApp: FirestoreDatabase | FirebaseApp,
authProvider: Provider<FirebaseAuthInternalName>,
persistenceProvider: ComponentProvider = new MemoryComponentProvider()
componentProvider: ComponentProvider = new MemoryComponentProvider()
) {
if (typeof (databaseIdOrApp as FirebaseApp).options === 'object') {
// This is very likely a Firebase app object
Expand All @@ -313,7 +313,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
this._credentials = new EmptyCredentialsProvider();
}

this._componentProvider = persistenceProvider;
this._componentProvider = componentProvider;
this._settings = new FirestoreSettings({});
this._dataReader = this.createDataReader(this._databaseId);
}
Expand Down
105 changes: 67 additions & 38 deletions packages/firestore/src/core/component_provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import {
SharedClientState,
WebStorageSharedClientState
} from '../local/shared_client_state';
import { LocalStore } from '../local/local_store';
import { SyncEngine } from './sync_engine';
import { LocalStore, MultiTabLocalStore } from '../local/local_store';
import { MultiTabSyncEngine, SyncEngine } from './sync_engine';
Copy link
Contributor

Choose a reason for hiding this comment

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

This review doesn't include MultiTabSyncEngine but I have some thoughts on how to move SharedClientState and related properties into it. Most of these options will make the code ugly, awkward or might not be viable but want to throw it out there anyway.

In any method in SyncEngine that has multi-tab logic is it possible to refactor it to not be in the middle? For example removeAndCleanupTarget() only has one line at the top. Could you do that to everything else? If so, you could rely on method overloading in MultiTabSyncEngine to do pre/post processing and call the base class method in the appropriate place. If the logic can't be split it might be worth overloading the method entirely and relying on unit tests to ensure consistency.

Alternatively, in methods where you need a return value from SharedClientState you can modify the method to have an optional callback parameter that usually gives you a default value but in MultiTabSyncEngine you can overload the method and call into the base with an argument for the callback that gives you a real result. You can do the same for notification only methods OR raise an event that MultiTabSyncEngine can subscribe to. The latter is super weird but eliminates the reference entirely without needing a stub implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked through all the calls to SharedClientState to see if we can improve the logic and remove some of the tight coupling. Unfortunately, I think in almost all cases the current logic is simpler than what we would have if we pulled out the code:

  • If we overwrite the methods to extract one line, it creates an interaction model that is much harder to follow. While it is relatively straightforward to pull out the first or last line in some of these methods, it essentially hides these lines from view if the use is not aware that they are overwritten in MultiTabSyncEngine.
  • In some cases (like removeAndCleanupTarget()) this is not actually all that bad - but it creates some inconsistencies between target allocation and target cleanup. I would rather keep all of this code in one place.

I did create an overwrite for applyOnlineStateChange. This method has different behavior for Multi-Tab clients and has no "mirrored" behavior in SyncEngine.

import { RemoteStore } from '../remote/remote_store';
import { EventManager } from './event_manager';
import { AsyncQueue } from '../util/async_queue';
Expand Down Expand Up @@ -81,7 +81,7 @@ export interface ComponentProvider {
* Provides all components needed for Firestore with in-memory persistence.
* Uses EagerGC garbage collection.
*/
export class MemoryComponentProvider {
export class MemoryComponentProvider implements ComponentProvider {
persistence!: Persistence;
sharedClientState!: SharedClientState;
localStore!: LocalStore;
Expand All @@ -106,24 +106,12 @@ export class MemoryComponentProvider {
OnlineStateSource.SharedClientState
);
this.remoteStore.syncEngine = this.syncEngine;
this.sharedClientState.syncEngine = this.syncEngine;

await this.localStore.start();
await this.sharedClientState.start();
await this.remoteStore.start();
await this.localStore.start();

// NOTE: This will immediately call the listener, so we make sure to
// set it after localStore / remoteStore are started.
await this.persistence.setPrimaryStateListener(async isPrimary => {
await this.syncEngine.applyPrimaryState(isPrimary);
if (this.gcScheduler) {
if (isPrimary && !this.gcScheduler.started) {
this.gcScheduler.start(this.localStore);
} else if (!isPrimary) {
this.gcScheduler.stop();
}
}
});
await this.remoteStore.applyPrimaryState(this.syncEngine.isPrimaryClient);
}

createEventManager(cfg: ComponentConfiguration): EventManager {
Expand All @@ -149,7 +137,7 @@ export class MemoryComponentProvider {
!cfg.persistenceSettings.durable,
'Can only start memory persistence'
);
return new MemoryPersistence(cfg.clientId, MemoryEagerDelegate.factory);
return new MemoryPersistence(MemoryEagerDelegate.factory);
}

createRemoteStore(cfg: ComponentConfiguration): RemoteStore {
Expand Down Expand Up @@ -192,13 +180,58 @@ export class MemoryComponentProvider {
* Provides all components needed for Firestore with IndexedDB persistence.
*/
export class IndexedDbComponentProvider extends MemoryComponentProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unable to comment on lines of code outside of what's changed however I see a few high level things:

  1. MemoryComponentProvider should explicitly implicitly implement ComponentProvider. Currently it doesn't.
  2. I think these assertions provide the correct guarantee with minimal lines of code but I wonder if instead you modified the ComponentProvider interface to take generic arguments for every member with defaults and then overrode it in MemoryComponentProvider and again here it would get you compile time safety and let you eliminate these assertions. It would be a large list of generic constraints which comes with its own problems but might be worth thinking about.

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Fixed (also renamed the constructor argument in FirestoreClient).
  2. I opted not to go down the route and make this generic, but I did overwrite some of the members and use more explicit types. This gets rid of some of the asserts. Let me know if that is enough.

As part of the Master merge, I also had to fix the spec tests to be aware of the differences between the different persistence implementations.

persistence!: IndexedDbPersistence;

// TODO(tree-shaking): Create an IndexedDbComponentProvider and a
// MultiTabComponentProvider. The IndexedDbComponentProvider should depend
// on LocalStore and SyncEngine.
localStore!: MultiTabLocalStore;
syncEngine!: MultiTabSyncEngine;

async initialize(cfg: ComponentConfiguration): Promise<void> {
await super.initialize(cfg);

// NOTE: This will immediately call the listener, so we make sure to
// set it after localStore / remoteStore are started.
await this.persistence.setPrimaryStateListener(async isPrimary => {
await (this.syncEngine as MultiTabSyncEngine).applyPrimaryState(
isPrimary
);
if (this.gcScheduler) {
if (isPrimary && !this.gcScheduler.started) {
this.gcScheduler.start(this.localStore);
} else if (!isPrimary) {
this.gcScheduler.stop();
}
}
});
}

createLocalStore(cfg: ComponentConfiguration): LocalStore {
return new MultiTabLocalStore(
this.persistence,
new IndexFreeQueryEngine(),
cfg.initialUser
);
}

createSyncEngine(cfg: ComponentConfiguration): SyncEngine {
const syncEngine = new MultiTabSyncEngine(
this.localStore,
this.remoteStore,
this.sharedClientState,
cfg.initialUser,
cfg.maxConcurrentLimboResolutions
);
if (this.sharedClientState instanceof WebStorageSharedClientState) {
this.sharedClientState.syncEngine = syncEngine;
}
return syncEngine;
}

createGarbageCollectionScheduler(
cfg: ComponentConfiguration
): GarbageCollectionScheduler | null {
debugAssert(
this.persistence instanceof IndexedDbPersistence,
'IndexedDbComponentProvider should provide IndexedDBPersistence'
);
const garbageCollector = this.persistence.referenceDelegate
.garbageCollector;
return new LruScheduler(garbageCollector, cfg.asyncQueue);
Expand All @@ -214,27 +247,23 @@ export class IndexedDbComponentProvider extends MemoryComponentProvider {
cfg.databaseInfo
);
const serializer = cfg.platform.newSerializer(cfg.databaseInfo.databaseId);
return IndexedDbPersistence.createIndexedDbPersistence({
allowTabSynchronization: cfg.persistenceSettings.synchronizeTabs,
return new IndexedDbPersistence(
cfg.persistenceSettings.synchronizeTabs,
persistenceKey,
clientId: cfg.clientId,
platform: cfg.platform,
queue: cfg.asyncQueue,
cfg.clientId,
cfg.platform,
LruParams.withCacheSize(cfg.persistenceSettings.cacheSizeBytes),
cfg.asyncQueue,
serializer,
lruParams: LruParams.withCacheSize(
cfg.persistenceSettings.cacheSizeBytes
),
sequenceNumberSyncer: this.sharedClientState
});
this.sharedClientState
);
}

createSharedClientState(cfg: ComponentConfiguration): SharedClientState {
debugAssert(
cfg.persistenceSettings.durable,
'Can only start durable persistence'
);

if (cfg.persistenceSettings.synchronizeTabs) {
if (
cfg.persistenceSettings.durable &&
cfg.persistenceSettings.synchronizeTabs
) {
if (!WebStorageSharedClientState.isAvailable(cfg.platform)) {
throw new FirestoreError(
Code.UNIMPLEMENTED,
Expand Down
Loading