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: pool.recycleWorkers() method #63

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

AriPerkkio
Copy link
Member

test/isolation.test.ts Outdated Show resolved Hide resolved
@AriPerkkio AriPerkkio force-pushed the feat/task-level-isolation branch from 15f9c65 to 89df201 Compare June 30, 2023 09:25
@AriPerkkio
Copy link
Member Author

@sheremet-va could you test if this fixes your PR on Vitest's side?

I quickly tested this with my reproduction case from vitest-dev/vitest#3659 and it seems to work nicely. No perf issues noticable.

        // always run environments isolated between each other
        for (const env of envs) {
+         if (envs.indexOf(env) !== 0)
+           await pool.recycleWorkers()

@sheremet-va
Copy link
Member

sheremet-va commented Jun 30, 2023

+         if (envs.indexOf(env) !== 0)

Shouldn't it always recycle? There is a possibility that some tests were running in multithreaded, so there are already some workers running (if you have a workspace project with isolate: false and another with isolate: true)

We also need a test for this. I think we already do, but looks like it's not correct

@AriPerkkio AriPerkkio force-pushed the feat/task-level-isolation branch from 89df201 to ddb3fed Compare June 30, 2023 09:39
@AriPerkkio
Copy link
Member Author

Shouldn't it always recycle?

My reproduction case was simple enough that it did not create conflicts there. But sure, it's OK to set pool to recycle when ever needed. It's not that slow to create new set of workers.

@AriPerkkio AriPerkkio changed the title feat: task level isolation feat: pool.recycleWorkers() method Jun 30, 2023
@sheremet-va
Copy link
Member

@sheremet-va could you test if this fixes your PR on Vitest's side?

Can confirm that tests in vitest-dev/vitest#3491 are passing

@AriPerkkio AriPerkkio marked this pull request as ready for review June 30, 2023 10:22
@AriPerkkio AriPerkkio requested a review from Aslemammad June 30, 2023 11:00
@AriPerkkio
Copy link
Member Author

We also need a test for this. I think we already do, but looks like it's not correct

vitest-dev/vitest@7d8413a

@AriPerkkio AriPerkkio force-pushed the feat/task-level-isolation branch 3 times, most recently from 47f22c8 to 992b957 Compare July 2, 2023 15:09
test/isolation.test.ts Show resolved Hide resolved
@AriPerkkio AriPerkkio force-pushed the feat/task-level-isolation branch from 992b957 to 0fa1fec Compare July 3, 2023 06:22
@Aslemammad Aslemammad merged commit 9150dbd into tinylibs:main Jul 5, 2023
@AriPerkkio AriPerkkio deleted the feat/task-level-isolation branch July 5, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: task level isolation
3 participants