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

fix: set correct workerId in runtime: 'child_process' #81

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

alisd23
Copy link
Contributor

@alisd23 alisd23 commented Jan 17, 2024

First attempt at using the correct threadId/workerId for the worker when in child_process mode.

  • Set worker data as required in init options as it seems to be always defined.
  • Pass internal (correct) tinypool workerId down to fork with process.env
  • Set correct fork state process.__tinypool_state__.workerId from the env variable

@alisd23
Copy link
Contributor Author

alisd23 commented Jan 17, 2024

I couldn't think of a better way to test the correct threadId so I decide to just check that the ID equals 1 exactly, which is more precise. I can't imagine that internal ID strategy would change anytime soon so I think it's probably fine?

Copy link
Member

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Impressive how fast you were able to set up a PR. 🚀

@Aslemammad this PR can be considered as semver patch. The process.__tinypool_state__.workerId was always supposed to be Tinypool's workerId and not the process.id as it was set.

@AriPerkkio AriPerkkio requested a review from Aslemammad January 17, 2024 18:24
@AriPerkkio AriPerkkio changed the title fix: use internal workerId for child_process runtime fix: set correct workerId in runtime: 'child_process' Jan 17, 2024
@Aslemammad
Copy link
Member

@AriPerkkio @alisd23 Thank you so much for the effort folks!

@Aslemammad Aslemammad merged commit 0eb5e0c into tinylibs:main Jan 17, 2024
6 checks passed
@alisd23
Copy link
Contributor Author

alisd23 commented Jan 17, 2024

Thanks both for the help and getting this through so quick! 👍

@alisd23
Copy link
Contributor Author

alisd23 commented Jan 17, 2024

Final thing, @Aslemammad when can we expect a release? Looks like releases are done manually is that right?

@Aslemammad
Copy link
Member

Final thing, @Aslemammad when can we expect a release? Looks like releases are done manually is that right?

Yea sure, I'm just going to give an access to @AriPerkkio so he can releases.

@AriPerkkio
Copy link
Member

Thanks! 🙌

I'll do release tomorrow and do some testing with Vitest.

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