feat: add concurrency safety #99
Conversation
Codecov Report
@@ Coverage Diff @@
## master #99 +/- ##
==========================================
+ Coverage 99.75% 99.78% +0.02%
==========================================
Files 5 5
Lines 409 460 +51
Branches 70 79 +9
==========================================
+ Hits 408 459 +51
Misses 1 1
Continue to review full report at Codecov.
|
src/adapter.ts
Outdated
|
|
||
| public waitForPending(logger: Logger): Promise<boolean> { | ||
| let wasPending = false | ||
| return new Promise<boolean>(async (resolve, reject) => { |
There was a problem hiding this comment.
making the Promise resolver async will swallow any exception that happen within it
src/adapter.ts
Outdated
| let wasPending = false | ||
| return new Promise<boolean>(async (resolve, reject) => { | ||
| // fail after 10 min | ||
| const timeout = setTimeout(() => reject(new PendingMigrationTimedOutError()), 1000 * 60 * 10) |
There was a problem hiding this comment.
If you just want to fail the Promise after 10min, I would use Promise.race() to race the timeout Promise against the operation Promise
src/adapters/postgres.ts
Outdated
| public async finishMigrationTask(task: Task): Promise<void> { | ||
| const { rows } = await this.client.query(SQL` | ||
| UPDATE merkel_meta SET "applied_at" = ${task.appliedAt}, "head" = ${ | ||
| task.head ? task.head.sha1 : null |
There was a problem hiding this comment.
Save these in variables so it doesn't get wrapped ugly?
src/cli.ts
Outdated
| const tasks = await Promise.all( | ||
| argv.migrations!.map(async name => { | ||
| const task = new Task({ type, migration: new Migration(name), head }) | ||
| await adapter.beginMigrationTask(task) |
There was a problem hiding this comment.
Are these run in parallel intentionally? Won't they output log statements etc?
| logger.log(' Success\n') | ||
| while (true) { | ||
| logger.log(this.toString()) | ||
| const tasks = this.newCommits.reduce<Task[]>((prev, next) => prev.concat(next.tasks), []) |
There was a problem hiding this comment.
Why can't this be done anymore in a nested for loop?
There was a problem hiding this comment.
it is below, but we need all of them before the migrations to create the pending entries
There was a problem hiding this comment.
wouldn't that be possible with a nested for loop too?
src/test/e2e.test.ts
Outdated
| await adapter.beginMigrationTask({ ...status.newCommits[0].tasks[0], head: await getHead() } as Task) | ||
| let shouldHaveFinished = false | ||
| // begin execution, this should wait | ||
| const promise = new Promise<void>(async (resolve, reject) => { |
There was a problem hiding this comment.
This swallows exceptions, you can make an async IIFE
src/adapter.ts
Outdated
| (async (): Promise<boolean> => { | ||
| // fail after 10 min | ||
| let interval: NodeJS.Timer | undefined | ||
| while (true) { |
There was a problem hiding this comment.
I don't see an exit condition for this loop
There was a problem hiding this comment.
there is only the return on l 60
don you think inf retries are too many? 😁
There was a problem hiding this comment.
But what in the case of the timeout? This loop would keep going
src/adapters/postgres.ts
Outdated
| SET "applied_at" = ${task.appliedAt}, | ||
| "head" = ${head}, | ||
| "commit" = ${commit} | ||
| WHERE "id" = ${task.id} |
There was a problem hiding this comment.
align statements like in other queries?
src/adapters/postgres.ts
Outdated
| public async finishMigrationTask(task: Task): Promise<void> { | ||
| const head = task.head ? task.head.sha1 : null | ||
| const commit = task.commit ? task.commit.sha1 : null | ||
| const { rows } = await this.client.query(SQL` |
eseliger
force-pushed
the
pending-migrations
branch
from
a7b9ff8
to
a466959
Compare
src/adapter.ts
Outdated
| @@ -14,6 +16,11 @@ export class UnsupportedDialectError extends Error { | |||
| } | |||
| } | |||
|
|
|||
| export class PendingMigrationTimedOutError extends Error { | |||
| /* istanbul ignore next */ | |||
| public name = 'PendingMigrationTimedOutError' | |||
There was a problem hiding this comment.
should be a string literal type and readonly
eseliger
force-pushed
the
pending-migrations
branch
from
a466959
to
4155f37
Compare
| }) | ||
| if (!answer.continue) { | ||
| process.exit(0) | ||
| while (true) { |
There was a problem hiding this comment.
Should this retry an infinite amount of times?
There was a problem hiding this comment.
hmm do you think it should fail?
src/cli.ts
Outdated
| } | ||
| // create pending tasks | ||
| for (const task of tasks) { | ||
| await adapter.beginMigrationTask(task) |
There was a problem hiding this comment.
Between this and waitForPendingMigrations(), could another process have started the same migration?
There was a problem hiding this comment.
hm yes, I added some failure recovery
eseliger
force-pushed
the
pending-migrations
branch
from
4155f37
to
2aa792d
Compare
eseliger
force-pushed
the
pending-migrations
branch
from
2aa792d
to
ee3b534
Compare
eseliger
force-pushed
the
pending-migrations
branch
from
437db56
to
2a1bbc2
Compare
|
🎉 This PR is included in version 0.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #58