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: workerId #30

Merged
merged 4 commits into from
Jun 23, 2022
Merged

feat: workerId #30

merged 4 commits into from
Jun 23, 2022

Conversation

Aslemammad
Copy link
Member

@Aslemammad Aslemammad commented Jun 21, 2022

This PR tries to solve this vitest issue where pool id can be greater than maxThreads! Therefore I'm implementing the workerId which would be generated for each worker and will not be greater than maxThreads

It can be imported from tinypool as workerId in the worker when running!

@Aslemammad Aslemammad requested a review from antfu June 21, 2022 20:45
@Aslemammad
Copy link
Member Author

@sheremet-va couldn't put you in the reviewers!

Copy link
Member

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

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

Also lockfile is kind of big for this changes, don't you think? 👀

src/index.ts Outdated
@@ -807,7 +836,7 @@ class ThreadPool {
taskInfo.workerInfo.taskInfos.delete(taskInfo.taskId)
if (!taskInfo.workerInfo.taskInfos.size) {
this._removeWorker(taskInfo.workerInfo)
this._ensureMaximumWorkers()
// this._ensureMaximumWorkers()
Copy link
Member

Choose a reason for hiding this comment

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

are you sure about this? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should not spawn workers, we should let the scheduler figures it out!

export * from './common'
export { Tinypool, Options }
export { Tinypool, Options, _workerId as workerId }
Copy link
Member

Choose a reason for hiding this comment

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

So, the only way I can get worker id is by importing tinypool inside a script that is running in a worker? Like this:

import { workerId } from 'tinypool'

Copy link
Member Author

Choose a reason for hiding this comment

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

or accessing process.__tinypool_state__?.workerId

@sheremet-va
Copy link
Member

Also some tests are failing

@antfu
Copy link
Collaborator

antfu commented Jun 22, 2022

Maybe we also need to remove v17 and add v18 for ci tests

@Aslemammad Aslemammad merged commit ed5347e into main Jun 23, 2022
@Aslemammad Aslemammad deleted the workerId branch June 23, 2022 03:20
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.

3 participants