fix: manage concurent initializations #3132
Conversation
commit: |
Co-authored-by: Farnabaz <[email protected]>
|
This is tested and works fine on https://kestra.io/docs |
There was a problem hiding this comment.
Thanks @elevatebart for the PR.
Using ready column as lock is smart 👍
I have two concerns regarding endless loop and invalid checksum (comments added)
And a suggestion to remove confusion from code
src/module.ts
Outdated
|
|
||
| collectionChecksum[collection.name] = hash(collectionDump[collection.name]) | ||
| collectionChecksum[collection.name] = hash(insertList) |
There was a problem hiding this comment.
With this change, the table definition is not used for hash generation. For example, if I change the default value of a number column in the table, we will still use the old table because the hash will not change.
| // if another request has already started the initialization of | ||
| // this version of this collection | ||
| // wait for it to finish | ||
| if (before.ready === false && before.version === integrityVersion) { | ||
| await new Promise((resolve) => { | ||
| const interval = setInterval(async () => { | ||
| const { ready } = await db.first<{ ready: boolean }>(`select ready from ${tables.info} where id = ?`, [`checksum_${collection}`]).catch(() => ({ ready: true })) | ||
| if (ready) { | ||
| clearInterval(interval) | ||
| resolve(0) | ||
| } | ||
| }, 200) | ||
| }) | ||
| return true | ||
| } |
There was a problem hiding this comment.
I think it is worth adding a timeout mechanism to this interval to make sure it always resolves.
Think of an edge case in which importing data fails when updating ready to true
There was a problem hiding this comment.
totally agree. I added a 5 min timeout if it does not respond. Then I throw.
| @@ -61,12 +61,30 @@ export async function checkAndImportDatabaseIntegrity(event: H3Event, collection | |||
| async function _checkAndImportDatabaseIntegrity(event: H3Event, collection: string, integrityVersion: string, config: RuntimeConfig['content']) { | |||
| const db = loadDatabaseAdapter(config) | |||
|
|
|||
| const before = await db.first<{ version: string }>(`select * from ${tables.info} where id = ?`, [`checksum_${collection}`]).catch(() => ({ version: '' })) | |||
| const before = await db.first<{ version: string, ready: boolean }>(`select * from ${tables.info} where id = ?`, [`checksum_${collection}`]).catch(() => ({ version: '', ready: true })) | |||
There was a problem hiding this comment.
At first glance it is hard to understand why read is true in catch.
it would be better if we return null or undefined in catch and move the interval logic inside if (before?.version) {.
const before = await db.first<{ version: string, ready: boolean }>(`select * from ${tables.info} where id = ?`, [`checksum_${collection}`]).catch(() => null)
if (before?.version) {
if (before.ready === true && before.version === integrityVersion) {
return true
}
if (before.ready === false && before.version === integrityVersion) {
// Loop
}
// Delete old version -- checksum exists but does not match with bundled checksum
await db.exec(`DELETE FROM ${tables.info} WHERE id = ?`, [`checksum_${collection}`])
}
|
I was in the process of testing it out, I hope I did not make any mistake... |
|
And... famous last words, he made a few mistake and broke nuxt content |
This reverts commit c351947.
🔗 Linked issue
closes #3129
❓ Type of change
📚 Description
Improve the concurent initialization of collections by waiting until one is finished to start another init.
Prevent double init
📝 Checklist