-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(bail): fix race condition in bail #8971
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
Changes from 15 commits
3c62089
d017c32
168da32
4dea735
9f1b0ae
c3fa105
add9b44
116c663
3fbb1bd
92c168a
fe7f953
e175b1e
17181e5
cd33b57
ce0d57e
e9c178f
687f205
d0fa753
7876307
a791707
6b53104
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ export class Pool { | |
| private activeTasks: ActiveTask[] = [] | ||
| private sharedRunners: PoolRunner[] = [] | ||
| private exitPromises: Promise<void>[] = [] | ||
| private _isCancelling: boolean = false | ||
| private cancellingPromise: Promise<void> | null = null | ||
|
|
||
| constructor(private options: Options, private logger: Logger) {} | ||
|
|
||
|
|
@@ -47,9 +47,9 @@ export class Pool { | |
| } | ||
|
|
||
| async run(task: PoolTask, method: 'run' | 'collect'): Promise<void> { | ||
| // Prevent new tasks from being queued during cancellation | ||
| if (this._isCancelling) { | ||
| throw new Error('[vitest-pool]: Cannot run tasks while pool is cancelling') | ||
| // Wait for any ongoing cancellation to complete before accepting new tasks | ||
| if (this.cancellingPromise) { | ||
| await this.cancellingPromise | ||
| } | ||
|
|
||
| // Every runner related failure should make this promise reject so that it's picked by pool. | ||
|
|
@@ -167,28 +167,30 @@ export class Pool { | |
| } | ||
|
|
||
| async cancel(): Promise<void> { | ||
| // Set flag to prevent new tasks from being queued | ||
| this._isCancelling = true | ||
|
Comment on lines
-170
to
-171
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pool should already have had all tests queued at this point. Or if there was another group, the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this was true, there wouldn't have been an issue #8852 |
||
| // Create a promise to track cancellation completion | ||
| const cancelPromise = (async () => { | ||
| const pendingTasks = this.queue.splice(0) | ||
|
|
||
| const pendingTasks = this.queue.splice(0) | ||
| if (pendingTasks.length) { | ||
| const error = new Error('Cancelled') | ||
| pendingTasks.forEach(task => task.resolver.reject(error)) | ||
| } | ||
|
|
||
| if (pendingTasks.length) { | ||
| const error = new Error('Cancelled') | ||
| pendingTasks.forEach(task => task.resolver.reject(error)) | ||
| } | ||
| const activeTasks = this.activeTasks.splice(0) | ||
| await Promise.all(activeTasks.map(task => task.cancelTask())) | ||
|
|
||
| const activeTasks = this.activeTasks.splice(0) | ||
| await Promise.all(activeTasks.map(task => task.cancelTask())) | ||
| const sharedRunners = this.sharedRunners.splice(0) | ||
| await Promise.all(sharedRunners.map(runner => runner.stop())) | ||
|
|
||
| const sharedRunners = this.sharedRunners.splice(0) | ||
| await Promise.all(sharedRunners.map(runner => runner.stop())) | ||
| await Promise.all(this.exitPromises.splice(0)) | ||
|
|
||
| await Promise.all(this.exitPromises.splice(0)) | ||
| this.workerIds.forEach((_, id) => this.freeWorkerId(id)) | ||
|
|
||
| this.workerIds.forEach((_, id) => this.freeWorkerId(id)) | ||
| this.cancellingPromise = null | ||
nicojs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| })() | ||
|
|
||
| // Reset flag after cancellation completes | ||
| this._isCancelling = false | ||
| this.cancellingPromise = cancelPromise | ||
| await cancelPromise | ||
| } | ||
|
|
||
| async close(): Promise<void> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import { expect, test } from 'vitest' | ||
|
|
||
| test('adds two numbers', () => { | ||
| expect(2 + 3).toBe(5) | ||
| }) | ||
|
|
||
| test('fails adding two numbers', () => { | ||
| expect(2 + 3).toBe(6) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { resolve } from 'pathe' | ||
| import { expect, test } from 'vitest' | ||
| import { createVitest } from 'vitest/node' | ||
nicojs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| test('cancels previous run before starting new one', async () => { | ||
| const results: string[] = [] | ||
|
|
||
| const vitest = await createVitest('test', { | ||
| root: resolve(import.meta.dirname, '../fixtures/bail-race'), | ||
| bail: 1, | ||
| maxWorkers: 1, | ||
| watch: false, | ||
| reporters: [{ | ||
| onTestCaseResult(testCase) { | ||
| const result = testCase.result() | ||
|
|
||
| results.push(`${result.state}${result.errors ? `: ${result.errors?.[0].message}` : ''}`) | ||
| }, | ||
| }], | ||
| }) | ||
|
|
||
| let rounds = 0 | ||
|
|
||
| while (vitest.state.errorsSet.size === 0) { | ||
| await vitest.start() | ||
|
|
||
| if (rounds >= 2) { | ||
| break | ||
| } | ||
|
|
||
| rounds++ | ||
| } | ||
|
|
||
| expect(results).toMatchInlineSnapshot(` | ||
| [ | ||
nicojs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "passed", | ||
| "failed: expected 5 to be 6 // Object.is equality", | ||
| "passed", | ||
| "failed: expected 5 to be 6 // Object.is equality", | ||
| "passed", | ||
| "failed: expected 5 to be 6 // Object.is equality", | ||
| ] | ||
| `) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this change will start some previous run's tests too. I have no idea why
this._isCancellingwas required before - @sheremet-va I think that's something you added?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment explains it:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These kinds of checks exist to catch any unintended behaviours
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we just
awaithere and then let it continue, doesn't it mean it's going to continue therunit was intended the cancel? Should itreturninstead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that was the previous cancellation (because of
bail). So it needs to wait for that to finish.