Skip to content
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

feat: add project.close() #375

Merged
merged 22 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8adec47
initial implementation of project.close()
Nov 13, 2023
7529dc6
add `close` to Datastore, use `coreManager.close()`
Nov 13, 2023
356d4ee
add sqlite as a private field, `.close()` it on `MapeoProject.close()`
Nov 13, 2023
1df497d
close dataStores in parallel
Nov 15, 2023
e3c395c
await #coreManager.close and dataStore promises
Nov 16, 2023
0e7ce31
update multi-core-indexer to alpha8
Nov 16, 2023
1062410
update lock
Nov 16, 2023
3e33597
Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/p…
Nov 16, 2023
ef7f93e
fix package-lock, add tests to close
Nov 21, 2023
bd88106
fix `.getMany` test for `project.close()`
Nov 21, 2023
904ee40
add tests for creating project after `project.close()`
Nov 21, 2023
f218f09
Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/p…
Nov 21, 2023
6774279
Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/p…
Dec 4, 2023
57ba9fc
* remove 'multiCoreIndexer.removeAllListener()' (the class is already
Dec 4, 2023
5fee846
remove cached project in manager after closing project
achou11 Dec 5, 2023
1ad7057
remove added listeners in mapeo project after close
achou11 Dec 5, 2023
a1b742e
[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.3 (#404)
optic-release-automation[bot] Dec 5, 2023
c286486
Revert "[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.3 (#404)"
achou11 Dec 5, 2023
d54311b
Merge branch 'main' into feat/projectClose
achou11 Dec 5, 2023
fd83222
update flaky sync e2e test now that project.close() is implemented
achou11 Dec 5, 2023
169af66
const instead of let in close() method
achou11 Dec 5, 2023
02ba36e
fix: close cores after indexing is closed
gmaclennan Dec 7, 2023
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
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
"hyperswarm": "4.4.1",
"magic-bytes.js": "^1.0.14",
"map-obj": "^5.0.2",
"multi-core-indexer": "1.0.0-alpha.7",
"multi-core-indexer": "1.0.0-alpha.8",
"p-defer": "^4.0.0",
"p-timeout": "^6.1.2",
"patch-package": "^8.0.0",
Expand Down
5 changes: 5 additions & 0 deletions src/datastore/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,9 @@ export class DataStore extends TypedEmitter {
if (!block) throw new Error('Not Found')
return block
}

async close() {
this.#coreIndexer.removeAllListeners()
Copy link
Member

Choose a reason for hiding this comment

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

should only remove listeners that this class has added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. The MultiCoreIndexer is already doing that, so I'll just remove that line

Copy link
Member

Choose a reason for hiding this comment

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

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

await this.#coreIndexer.close()
}
}
23 changes: 19 additions & 4 deletions src/mapeo-project.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export class MapeoProject {
#coreOwnership
#capabilities
#ownershipWriteDone
#sqlite
#memberApi
#iconApi
#syncApi
Expand Down Expand Up @@ -99,8 +100,8 @@ export class MapeoProject {
this.#projectId = projectKeyToId(projectKey)

///////// 1. Setup database
const sqlite = new Database(dbPath)
const db = drizzle(sqlite)
this.#sqlite = new Database(dbPath)
const db = drizzle(this.#sqlite)
migrate(db, {
migrationsFolder: new URL('../drizzle/project', import.meta.url).pathname,
})
Expand All @@ -123,7 +124,7 @@ export class MapeoProject {
projectKey,
keyManager,
storage: coreManagerStorage,
sqlite,
sqlite: this.#sqlite,
logger: this.#l,
})

Expand All @@ -137,7 +138,7 @@ export class MapeoProject {
deviceInfoTable,
iconTable,
],
sqlite,
sqlite: this.#sqlite,
getWinner,
mapDoc: (doc, version) => {
switch (doc.schemaName) {
Expand Down Expand Up @@ -353,6 +354,20 @@ export class MapeoProject {
await Promise.all([this.#coreManager.ready(), this.#ownershipWriteDone])
}

/**
*/
async close() {
this.#l.log('closing project %h', this.#projectId)
await this.#coreManager.close()
let dataStorePromises = []
for (const dataStore of Object.values(this.#dataStores)) {
dataStorePromises.push(dataStore.close())
}
await Promise.all(dataStorePromises)

this.#sqlite.close()
}

/**
* @param {import('multi-core-indexer').Entry[]} entries
* @param {{projectIndexWriter: IndexWriter, sharedIndexWriter: IndexWriter}} indexWriters
Expand Down
56 changes: 56 additions & 0 deletions test-e2e/project-crud.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,62 @@ test('CRUD operations', async (t) => {
'expected values returns from getMany()'
)
})
t.test('create, close and then create, update', async (st) => {
const projectId = await manager.createProject()
const project = await manager.getProject(projectId)
const values = new Array(5).fill(null).map(() => {
return getUpdateFixture(value)
})
for (const value of values) {
// @ts-ignore
await project[schemaName].create(value)
}
// @ts-ignore
const written = await project[schemaName].create(value)
await project.close()

await st.exception(async () => {
const updateValue = getUpdateFixture(value)
// @ts-ignore
await project[schemaName].update(written.versionId, updateValue)
}, 'should fail updating since the project is already closed')

await st.exception(async () => {
for (const value of values) {
// @ts-ignore
await project[schemaName].create(value)
}
}, 'should fail creating since the project is already closed')

// @ts-ignore
await st.exception.all(async () => {
await project[schemaName].getMany()
}, 'should fail getting since the project is already closed')
})
t.test(
'create project, close, then re-create a project and create',
async (st) => {
// create project
const projectId = await manager.createProject()
const project = await manager.getProject(projectId)
// 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)
})

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

@achou11 achou11 Dec 5, 2023

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

see 5fee846 for relevant change

for (const value of newValues) {
await st.execution(
// @ts-ignore
await newProject[schemaName].create(value),
'create after `project.close()` and creating new project'
)
}
}
)
}
})

Expand Down