Skip to content

Implement limbo resolution throttling. #2790

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 7 commits into from
Mar 30, 2020
Merged

Implement limbo resolution throttling. #2790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d8d7293
Implement limbo resolution throttling.
dconeybe Mar 16, 2020
5900aa8
Implement code review feedback from @wilhuff, round 1
dconeybe Mar 26, 2020
f0a2077
Implement code review feedback from @wilhuff, round 2
dconeybe Mar 30, 2020
45802c3
[AUTOMATED]: License Headers
dconeybe Mar 30, 2020
545fa89
Fix spelling error in method name: validateEnqeuedLimboDocs -> valida…
dconeybe Mar 30, 2020
abfa6fb
Merge remote-tracking branch 'origin/master' into LimboThrottling
dconeybe Mar 30, 2020
4c1a20c
Make maxConcurrentLimboResolutions a required parameter of the SyncEn…
dconeybe Mar 30, 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: 3 additions & 1 deletion packages/firestore/src/core/firestore_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import { OnlineState, OnlineStateSource } from './types';
import { ViewSnapshot } from './view_snapshot';

const LOG_TAG = 'FirestoreClient';
const MAX_CONCURRENT_LIMBO_RESOLUTIONS = 100;

/** DOMException error code constants. */
const DOM_EXCEPTION_INVALID_STATE = 11;
Expand Down Expand Up @@ -369,7 +370,8 @@ export class FirestoreClient {
this.localStore,
this.remoteStore,
this.sharedClientState,
user
user,
MAX_CONCURRENT_LIMBO_RESOLUTIONS
);

this.sharedClientState.onlineStateHandler = sharedClientStateOnlineStateChangedHandler;
Expand Down
93 changes: 69 additions & 24 deletions packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,23 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
q.canonicalId()
);
private queriesByTarget: { [targetId: number]: Query[] } = {};
private limboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
/**
* The keys of documents that are in limbo for which we haven't yet started a
* limbo resolution query.
*/
private enqueuedLimboResolutions: DocumentKey[] = [];
/**
* Keeps track of the target ID for each document that is in limbo with an
* active target.
*/
private activeLimboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
DocumentKey.comparator
);
private limboResolutionsByTarget: {
/**
* Keeps track of the information about an active limbo resolution for each
* active target ID that was started for the purpose of limbo resolution.
*/
private activeLimboResolutionsByTarget: {
[targetId: number]: LimboResolution;
} = {};
private limboDocumentRefs = new ReferenceSet();
Copy link
Contributor

Choose a reason for hiding this comment

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

Many of the existing variable names are now ambiguous because it's not clear if they apply to active limbo resolutions, inactive limbo resolutions, or all. I suggest prefixing all these names to clearly indicate which kind each is.

Other ideas for ways to clarify this:

  • active vs pending (though "pending" could also possibly be read to mean the resolution for which we're waiting on a result)
  • active vs deferred?

If we document this distinction though I think any pair could be good and an improvement. WDYT?

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 like the names "enqueued" and "active" for the states of documents that are in limbo. Since a limbo resolution is "active" if and only if it is associated with a target, the limboTargetsByKey and limboResolutionsByTarget imply that they only apply to "active" limbo resolutions. Similarly, a limbo resolution is "enqueued" if and only if its document key is in the limboListenQueue. My sense is that adding "active" and "inactive" to these variable names will be confusing because they have no counterpart for the opposite state.

Copy link
Contributor

Choose a reason for hiding this comment

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

variable names will be confusing because they have no counterpart for the opposite state

That's an interesting perspective but I think the reasoning is kind of subtle: that because only active limbo resolutions can have a target the active-ness is self-explanatory. Would this have been self-evident to you a week ago?

My reasoning was that the prefix helps identify which variables are used in which state, and suggests which elements have to be modified as you transition between states. That is, this would have the effect of grouping these things together more than necessarily differentiating similar structures of the same type that might be used in the different states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I've added "active" and "enqueued" to all relevant variable names throughout this PR.

Expand All @@ -174,7 +187,8 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
private remoteStore: RemoteStore,
// PORTING NOTE: Manages state synchronization in multi-tab environments.
private sharedClientState: SharedClientState,
private currentUser: User
private currentUser: User,
private maxConcurrentLimboResolutions: number
) {}

// Only used for testing.
Expand Down Expand Up @@ -400,7 +414,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
const changes = await this.localStore.applyRemoteEvent(remoteEvent);
// Update `receivedDocument` as appropriate for any limbo targets.
objUtils.forEach(remoteEvent.targetChanges, (targetId, targetChange) => {
const limboResolution = this.limboResolutionsByTarget[Number(targetId)];
const limboResolution = this.activeLimboResolutionsByTarget[
Number(targetId)
];
if (limboResolution) {
// Since this is a limbo resolution lookup, it's for a single document
// and it could be added, modified, or removed, but not a combination.
Expand Down Expand Up @@ -479,13 +495,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
// PORTING NOTE: Multi-tab only.
this.sharedClientState.updateQueryState(targetId, 'rejected', err);

const limboResolution = this.limboResolutionsByTarget[targetId];
const limboResolution = this.activeLimboResolutionsByTarget[targetId];
const limboKey = limboResolution && limboResolution.key;
if (limboKey) {
// Since this query failed, we won't want to manually unlisten to it.
// So go ahead and remove it from bookkeeping.
this.limboTargetsByKey = this.limboTargetsByKey.remove(limboKey);
delete this.limboResolutionsByTarget[targetId];
this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.remove(
limboKey
);
delete this.activeLimboResolutionsByTarget[targetId];
this.pumpEnqueuedLimboResolutions();

// TODO(klimt): We really only should do the following on permission
// denied errors, but we don't have the cause code here.
Expand Down Expand Up @@ -731,15 +750,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
private removeLimboTarget(key: DocumentKey): void {
// It's possible that the target already got removed because the query failed. In that case,
// the key won't exist in `limboTargetsByKey`. Only do the cleanup if we still have the target.
const limboTargetId = this.limboTargetsByKey.get(key);
const limboTargetId = this.activeLimboTargetsByKey.get(key);
if (limboTargetId === null) {
// This target already got removed, because the query failed.
return;
}

this.remoteStore.unlisten(limboTargetId);
this.limboTargetsByKey = this.limboTargetsByKey.remove(key);
delete this.limboResolutionsByTarget[limboTargetId];
this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.remove(key);
delete this.activeLimboResolutionsByTarget[limboTargetId];
this.pumpEnqueuedLimboResolutions();
}

private updateTrackedLimbos(
Expand Down Expand Up @@ -768,29 +788,54 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

private trackLimboChange(limboChange: AddedLimboDocument): void {
const key = limboChange.key;
if (!this.limboTargetsByKey.get(key)) {
if (!this.activeLimboTargetsByKey.get(key)) {
logDebug(LOG_TAG, 'New document in limbo: ' + key);
this.enqueuedLimboResolutions.push(key);
this.pumpEnqueuedLimboResolutions();
}
}

/**
* Starts listens for documents in limbo that are enqueued for resolution,
* subject to a maximum number of concurrent resolutions.
*
* Without bounding the number of concurrent resolutions, the server can fail
* with "resource exhausted" errors which can lead to pathological client
* behavior as seen in https://github.com/firebase/firebase-js-sdk/issues/2683.
*/
private pumpEnqueuedLimboResolutions(): void {
while (
this.enqueuedLimboResolutions.length > 0 &&
this.activeLimboTargetsByKey.size < this.maxConcurrentLimboResolutions
) {
const key = this.enqueuedLimboResolutions.shift()!;
const limboTargetId = this.limboTargetIdGenerator.next();
const query = Query.atPath(key.path);
this.limboResolutionsByTarget[limboTargetId] = new LimboResolution(key);
this.activeLimboResolutionsByTarget[limboTargetId] = new LimboResolution(
key
);
this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.insert(
key,
limboTargetId
);
this.remoteStore.listen(
new TargetData(
query.toTarget(),
Query.atPath(key.path).toTarget(),
limboTargetId,
TargetPurpose.LimboResolution,
ListenSequence.INVALID
)
);
this.limboTargetsByKey = this.limboTargetsByKey.insert(
key,
limboTargetId
);
}
}

// Visible for testing
currentLimboDocs(): SortedMap<DocumentKey, TargetId> {
return this.limboTargetsByKey;
activeLimboDocumentResolutions(): SortedMap<DocumentKey, TargetId> {
return this.activeLimboTargetsByKey;
}

// Visible for testing
enqueuedLimboDocumentResolutions(): DocumentKey[] {
return this.enqueuedLimboResolutions;
}

private async emitNewSnapsAndNotifyLocalStore(
Expand Down Expand Up @@ -936,12 +981,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

// PORTING NOTE: Multi-tab only.
private resetLimboDocuments(): void {
objUtils.forEachNumber(this.limboResolutionsByTarget, targetId => {
objUtils.forEachNumber(this.activeLimboResolutionsByTarget, targetId => {
this.remoteStore.unlisten(targetId);
});
this.limboDocumentRefs.removeAllReferences();
this.limboResolutionsByTarget = [];
this.limboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
this.activeLimboResolutionsByTarget = [];
this.activeLimboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
DocumentKey.comparator
);
}
Expand Down Expand Up @@ -1138,7 +1183,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
}

getRemoteKeysForTarget(targetId: TargetId): DocumentKeySet {
const limboResolution = this.limboResolutionsByTarget[targetId];
const limboResolution = this.activeLimboResolutionsByTarget[targetId];
if (limboResolution && limboResolution.receivedDocument) {
return documentKeySet().add(limboResolution.key);
} else {
Expand Down
Loading