-
Notifications
You must be signed in to change notification settings - Fork 67
Remove document from synchronizer when document is removed from repo #423
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
base: main
Are you sure you want to change the base?
Conversation
log(`removing document ${documentId}`) | ||
const docSynchronizer = this.docSynchronizers[documentId] | ||
if (docSynchronizer !== undefined) { | ||
this.peers.forEach(peerId => docSynchronizer.endSync(peerId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call endSync
first to allow for any cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @georgewsu -- thanks for the PR! I've been on holiday the last week and local-first conf is coming up this week so I'm going to be a bit time crunched for reviews but I see your patch and am looking forward to reviewing it soon. |
@@ -75,4 +75,27 @@ describe("CollectionSynchronizer", () => { | |||
|
|||
setTimeout(done) | |||
})) | |||
|
|||
it("removes document", () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including a test. @georgewsu can you think of a way we can test the memory usage, since that's a key motivation here?
Okay, I've given it a review & test and overall it seems good, but I think given that the primary motivation was freeing memory after delete I'd like to at least see if we can make some effort to demonstrate that it has the desired effect. If we can't automate it, @georgewsu, can you at least give me some steps I can run manually to validate that it's working? |
@@ -798,8 +797,8 @@ export class Repo extends EventEmitter<RepoEvents> { | |||
) | |||
} | |||
delete this.#handleCache[documentId] | |||
// TODO: remove document from synchronizer when removeDocument is implemented | |||
// this.synchronizer.removeDocument(documentId) | |||
delete this.#progressCache[documentId] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added call to delete from progressCache
Sounds good. I updated https://github.com/georgewsu/automerge-client-test to reproduce memory usage before/after.
Thanks! |
Confirmed steps to test from scratch:
|
Implement removal of a document when the document is removed from the repo.
This can happen in two cases:
-when the document is deleted
-when the document is removed from the handle cache
Specifically, this reference graph needs to be released in order to free up memory usage of the document:
-repo references synchronizer: CollectionSynchronizer
-synchronizer has record from documentId to docSynchronizer
-docSynchronizer has reference to DocHandle
Issue:
#424
Related issues:
#149
#358
#330