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

Add support for worker_threads #4

Closed
wants to merge 2 commits into from
Closed

Conversation

xvello
Copy link

@xvello xvello commented May 3, 2023

This is port of Blizzard/node-rdkafka#857 by @Twixes, that registers the module with NAN_MODULE_WORKER_ENABLED, for compatibility with worker_threads.

Testing

  • Existing test coverage confirms the main thread usage is unaffected
  • Additional test test/index.spec.js confirms that the module is now available in a worker thread

Compatibility

nan transparently falls back to the old API for versions older than 10. I tested it with node-v8.0.0-darwin-x64 (build and test run fine).

Do we need cleanup?

Looking at PRs on other libraries, I found that this one adds a call to node::AddEnvironmentCleanupHook to avoid leaking resources on worker exit. I'll need to check the codebase to see whether it's needed here.

@xvello
Copy link
Author

xvello commented May 3, 2023

Hello @robinfehr, and thanks for moving this fork forward!

We already moved one workload at @PostHog from kafkajs to this lib and measured order-of-magnitude performance improvements. The other workload we want to migrate uses worker threads, which are not currently supported. We'd appreciate a review on this if you can spare the time.

src/binding.cc Outdated Show resolved Hide resolved
@robinfehr
Copy link
Contributor

@xvello thanks for the PR!
much appreciated that you checked if it falls back correctly to NAN_MODULE if an older node version is present.

currently im not sure if we'd need AddEnvironmentCleanupHook - according to this:
nodejs/node@5c6cf30
we'd have to check for node version >= 11 instead of 10 though.
could you verify already whether that's needed?

@robinfehr robinfehr self-assigned this May 12, 2023
@robinfehr
Copy link
Contributor

a e2e test would be much appreciated.
we're in the process of moving the e2e tests to a new testing frame work which enables us to run tests much easier.

you can find them here
testsNew/e2e

@xvello
Copy link
Author

xvello commented May 15, 2023

Hello @robinfehr, thanks for your reply! I'll look into these two topics, and also update the README document the multi-threaded behaviours.
I do have a time-sensitive topic on my hands right now, but I'll hopefully circle back to this PR at the end of the month.

@robinfehr
Copy link
Contributor

no worries :) im the meantime i'll cleanup the README - long overdue :)

@xvello
Copy link
Author

xvello commented Nov 8, 2023

Hello @robinfehr, sorry for the noise. We decided to move away from worker threads so I'll be closing that PR for now. If we were to reverse that decision I'll add testing and reopen it. If anyone else needs it and want to push that PR to the finish line, feel free to reuse this code!

@xvello xvello closed this Nov 8, 2023
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