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 #1556: Reduce CPU usage on idle consumers (avoid infinite setTimeout loop with no inflight requests) #1667

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sergeyevstifeev
Copy link

@sergeyevstifeev sergeyevstifeev commented Feb 27, 2024

I've noticed that consumers spent an inordinate amount of time while idle on topics with no new messages. After setting up a sample consumer on an idle topic I arrived at the same conclusion as some of the commenters on Issue 1556 - scheduleCheckPendingRequests is eating all of the CPU. Debugging it further, the issue is:

scheduleCheckPendingRequests() {
    let scheduleAt = this.throttledUntil - Date.now() // throttledUntil is -1, so scheduleAt is negative
    if (!this.throttleCheckTimeoutId) {
      if (this.pending.length > 0) { // this branch isn't run since there's no pending requests, so scheduleAt stays negative
        scheduleAt = scheduleAt > 0 ? scheduleAt : CHECK_PENDING_REQUESTS_INTERVAL
      }
      this.throttleCheckTimeoutId = setTimeout(() => {
        this.throttleCheckTimeoutId = null
        this.checkPendingRequests()
      }, scheduleAt) // we setTimeout with negative scheduleAt, resulting in an infinite checkPendingRequests->scheduleCheckPendingRequests setTimeout loop
    }
  }

The fix addresses the infinite loop and reduces CPU consumption substantially on idle consumers. 🌱 Additionally, there is no need in CHECK_PENDING_REQUESTS_INTERVAL of 10ms if there is not throttling, so this is also removed. I looked at 1532 and I think the fix here should still address the issue that was being addressed in that PR - if there are pending requests we always schedule next check (Math.max(timeUntilUnthrottled, 0)), so there should be no regression.

Fixes #1556

@gpratte
Copy link

gpratte commented Aug 25, 2024

@sergeyevstifeev I see this cannot be merged because "This branch is out-of-date with the base branch". Are you going to fix this?

@sergeyevstifeev
Copy link
Author

@gpratte it doesn't seem like this repo is actively maintained anymore, so I don't really see a point.

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.

190% CPU hike on v2.2.4
2 participants