feat: add project.close()
#375
Conversation
for now: close coreManager cores for every namespace
There was a problem hiding this comment.
Would take a look at the tasks in the related issue as a starting point for things to address in this PR. Non-exhaustive list of tangible changes that probably need to happen:
- closing the project sqlite instance created in
MapeoProject - closing the indexer storage and core manager storage created in
MapeoProject - adding a
close()method (or something similar) to the followingCoreManagerDataStoreIndexWriter
Up to you if you want to separate into separate PRs! (probably makes sense to if it isn't too unwieldy)
Also needs tests (probably in e2e?) to test the following, as a minimum:
- subsequent method calls on the project related to the db/events fail after closing
- can re-create a project instance after closing it and call relevant methods without issue
the close method of `Datastore` basically `.removeAllListeners()` and `close()` over the `#coreIndexer` instance. I don't if that should be enough? But looking at the class, it seems so...
|
To close the // multicoreIndexer.close()
async close(){
...
this.#coreIndexStreams.map(indexStream => {
indexStream.storage.close()
})
}In the |
|
Yes this makes sense to me @tomasciccola. I think cleaner to just keep a set of storage instances on the parent class - we create the instance in the "add core" method. Storages need to be closed after the stream is closed (writes happen during the stream destroy). |
Its weird that we also create that storage on the constructor (see here). |
|
I've open an issue here to see approaches of exposing storage on the |
Since we're indexing multiple cores, there needs to be storage for index state for each core (index state is a bitfield). We create a stream for each core of all data that exists in the core and all data that is added to the core. The multi core index stream is then the merge of all those core index streams. Storage is created in two places because we add cores to the indexer in two places: the cores that exist when we first create the indexer, then any cores that are added after. |
|
I'm struggling to see how to close the |
Yes. See https://github.com/holepunchto/hypercore/blob/4096ec0b820a9bdcbb435a9848fa73b4825db33c/index.js#L436-L483 and https://github.com/holepunchto/hypercore/blob/4096ec0b820a9bdcbb435a9848fa73b4825db33c/lib/core.js#L827-L838
|
There was a problem hiding this comment.
One small change about removing event listeners - generally removeAllListeners() is a bad idea unless we're absolutely sure nothing else could have attached a listener, because if it has then this can lead to hard-to-track-down bugs. It's better to only remove listeners that a class has added within a class close() method.
I left a note about the tests too. However I don't think it's a priority to get strong test coverage for project.close() at this stage, because it is only used in tests. I would just delete the test that doesn't really test anything.
Also merge conflicts need resolved.
src/datastore/index.js
Outdated
| @@ -192,4 +192,9 @@ export class DataStore extends TypedEmitter { | |||
| if (!block) throw new Error('Not Found') | |||
| return block | |||
| } | |||
|
|
|||
| async close() { | |||
| this.#coreIndexer.removeAllListeners() | |||
There was a problem hiding this comment.
should only remove listeners that this class has added.
There was a problem hiding this comment.
Ok. The MultiCoreIndexer is already doing that, so I'll just remove that line
There was a problem hiding this comment.
added relevant change in 1ad7057 to remove the listeners added to the LocalPeers instance in the constructor. @gmaclennan lmk if this seems like the correct way to go about it
test-e2e/project-crud.js
Outdated
| // close it | ||
| await project.close() | ||
| // re create project | ||
| const newProjectId = await manager.createProject() | ||
| const newProject = await manager.getProject(newProjectId) | ||
| const newValues = new Array(5).fill(null).map(() => { | ||
| return getUpdateFixture(value) | ||
| }) | ||
|
|
There was a problem hiding this comment.
This isn't really testing anything - it's closing one project and creating a different one. Is this meant to test closing then re-opening a project? Then it should use the same projectId and check the data that was written before close is still there.
There was a problem hiding this comment.
Yeah, I realize this was the purpose of this tests. But I'm wondering how to achieve that.
If I close a project I'm also closing the storage, and there's not an 'project.open()' or smth similiar. For now I've commented that test...
There was a problem hiding this comment.
@tomasciccola all of the storage instances will be re-created when you call manager.getProject(...) (more specifically when a new mapeo project instance is created), but one thing that needs to be done is removing the cached project instances from the manager (#activeProjects) after closing the project, otherwise the getProject() call will return the instance with all of the closed storages. I have working changes that update the MapeoProject to emit a 'close' event that we use to do this when creating the project instances in the MapeoManager. can push those changes with an updated working test if that's helpful
removing added listeners) * comment crud test regarding closing and re-opening a project
|
Awesome, gonna merge this. thanks @achou11 and @gmaclennan for the help on fixing that! |
should close #227
I still need to figure out if we should create additional PRs to address closing different parts of a project (i.e. are there parts where we should expose a
close()method that hasn't been exposed yet?)