Skip to content

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 3 additions & 4 deletions packages/automerge-repo/src/Repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ export class Repo extends EventEmitter<RepoEvents> {
this.sharePolicy = sharePolicy ?? this.sharePolicy

this.on("delete-document", ({ documentId }) => {
// TODO Pass the delete on to the network
// synchronizer.removeDocument(documentId)
this.synchronizer.removeDocument(documentId)

if (storageSubsystem) {
storageSubsystem.removeDoc(documentId).catch(err => {
Expand Down Expand Up @@ -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]
Copy link
Contributor Author

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

this.synchronizer.removeDocument(documentId)
} else {
this.#log(
`WARN: removeFromCache called but doc undefined for documentId: ${documentId}`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,15 @@ export class CollectionSynchronizer extends Synchronizer {
})
}

// TODO: implement this
// eslint-disable-next-line @typescript-eslint/no-unused-vars
/** Removes a document and stops synchronizing them */
removeDocument(documentId: DocumentId) {
throw new Error("not implemented")
log(`removing document ${documentId}`)
const docSynchronizer = this.docSynchronizers[documentId]
if (docSynchronizer !== undefined) {
this.peers.forEach(peerId => docSynchronizer.endSync(peerId))
Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that in DocSynchronizer, endSync only removes each peer. This doesn't fully cover the delete protocol as described in issue #149.

However, the deletes below do address the cache eviction and retained memory usage issues:
#330
#358

}
delete this.docSynchronizers[documentId]
delete this.#docSetUp[documentId]
}

/** Adds a peer and maybe starts synchronizing with them */
Expand Down
23 changes: 23 additions & 0 deletions packages/automerge-repo/test/CollectionSynchronizer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,27 @@ describe("CollectionSynchronizer", () => {

setTimeout(done)
}))

it("removes document", () =>
Copy link
Member

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?

new Promise<void>((done, reject) => {
const handle = repo.create()
synchronizer.addDocument(handle)
synchronizer.addPeer("peer1" as PeerId)
// starts synchronizing document to peer
synchronizer.once("message", event => {
const { targetId, documentId } = event as SyncMessage
assert(targetId === "peer1")
assert(documentId === handle.documentId)
done()
})
// no message should be sent after removing document
synchronizer.once("message", () => {
reject(new Error("Should not have sent a message"))
})
assert(synchronizer.docSynchronizers[handle.documentId] !== undefined)
synchronizer.removeDocument(handle.documentId)
assert(synchronizer.docSynchronizers[handle.documentId] === undefined)
// removing document again should not throw an error
synchronizer.removeDocument(handle.documentId)
}))
})
6 changes: 6 additions & 0 deletions packages/automerge-repo/test/Repo.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -519,13 +519,19 @@ describe("Repo", () => {
const handle = repo.create({ foo: "bar" })
await repo.delete(handle.documentId)
assert(repo.handles[handle.documentId] === undefined)
assert(
repo.synchronizer.docSynchronizers[handle.documentId] === undefined
)
})

it("removeFromCache removes doc handle", async () => {
const { repo } = setup()
const handle = repo.create({ foo: "bar" })
await repo.removeFromCache(handle.documentId)
assert(repo.handles[handle.documentId] === undefined)
assert(
repo.synchronizer.docSynchronizers[handle.documentId] === undefined
)
})

it("removeFromCache for documentId not found", async () => {
Expand Down