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

Run consume logic in a separate thread #982

Merged

Conversation

GaryWilber
Copy link
Collaborator

The current consume method works like this:

  1. Schedules work on an existing node.js worker thread
  2. Runs an infinite loop on that thread until the consumer is closed
  3. Sleeps when it hits EOF and this is not configurable

That logic is not great. The while() loop is essentially blocking one of the 4 node.js worker threads from doing any other work because it took over with its infinite loop. You can confirm this yourself by starting an HTTP server & then calling the consume method. The HTTP server will not process any requests because it’s unable to schedule the work since that node.js worker thread is blocked – this repro’s 100% of the time for me on node.js 16 with a fastify webserver.

Some reading regarding node.js & it’s worker threads / thread pool to learn more:

This PR does the following:

  1. Create a separate thread for running the consume logic. This makes it so the consume while loop will not block any other node.js work, such as processing network requests
  2. Removed the EOF sleeps - those were slowing things down and the sleeps seemed unnecessary
  3. Allow setting consume timeout to 0. Before, setting it to 0 would end up making it set to the default time instead.

These changes result in a 30% latency reduction for my scenario. My application has been running with these changes for a while now and it has been stable.

This PR should be checked in as a squash commit due to the amount of commits.

@sharmakhushboo
Copy link
Collaborator

Ping @webmakersteve , @iradul, @eandersson , @BlizzTom

@RomainDECOSTER
Copy link

Hello,

I am currently experiencing the same issue.
I have 4 consumers listening and I am not longer able to use my express API.

Is it still planned to merge this PR?

Thank you in advance and thank you for the work you are doing.

Regards.

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