Skip to content

Remove local references from reference delegate #42

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 3 commits into from
Sep 24, 2018

Conversation

schmidt-sebastian
Copy link
Contributor

Fixes issue with GC that prevented local document removal for documents that had pending mutations.

Also renames applyBatchResult to applyWriteToRemoteDocuments to match Web.

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

IIUC, you will be adding the test for this in an upcoming PR?

@@ -558,7 +558,13 @@ public void releaseQuery(Query query) {
queryCache.updateQueryData(queryData);
}

localViewReferences.removeReferencesForId(queryData.getTargetId());
// We remove both locally edited documents and documents that are associated via the query
// cache.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add more to this comment. Something like: "We may have determined that a document is associated with a target, but if that hasn't been confirmed by the backend, the query cache won't know to remove the reference for those documents. Make sure we remove them here."

Or some variation on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment to:

// References for documents sent via Watch are automatically removed when we delete a
// query's target data from the reference delegate. Since this does not remove references
// for locally mutated documents, we have to remove the target associations for these
// documents manually.

@gsoltis gsoltis assigned schmidt-sebastian and unassigned gsoltis Sep 24, 2018
@schmidt-sebastian
Copy link
Contributor Author

FYI: I also added a unit test.

@schmidt-sebastian schmidt-sebastian merged commit 95703c5 into master Sep 24, 2018
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt-fixlru branch October 15, 2018 17:25
@firebase firebase locked and limited conversation to collaborators Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants