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

improve latency when a client is unblocked by module timer #9593

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

tzongw
Copy link
Contributor

@tzongw tzongw commented Oct 4, 2021

Describe the bug

  1. client block on command XREAD BLOCK 0 STREAMS mystream $
  2. in a module, calling XADD mystream * field value via lua from a timer callback
  3. client will receive response after some latency up to 100ms

Reason
When XADD signal the key mystream as ready, beforeSleep in next eventloop will call handleClientsBlockedOnKeys to unblock the client and add pending data to write but not actually install a write handler, so next redis will block in aeApiPoll up to 100ms given hz config as default 10, pending data will be sent in another next eventloop by handleClientsWithPendingWritesUsingThreads.

Calling handleClientsBlockedOnKeys before handleClientsWithPendingWritesUsingThreads in beforeSleep solves the problem.

Related issues #8125 #7880 #7903

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Oct 4, 2021
@oranagra
Copy link
Member

oranagra commented Oct 4, 2021

@tzongw thank you.
i see you mentioned the XADD was from a module via a timer event, but as far as i can tell the exact same problem will happen for a normal client sending XADD.
the 100ms delay will happen when redis is not experiencing any traffic (i.e. if someone sends a high rate of traffic or event PINGs) it'll dramatically shorten the latency.
anything i'm missing?

@tzongw
Copy link
Contributor Author

tzongw commented Oct 4, 2021

@oranagra I didn't see the same problem happen for a normal client sending XADD. I notice that handleClientsBlockedOnKeys is called in processCommand, so handleClientsWithPendingWritesUsingThreads in beforeSleep in next eventloop will send the pending data, no aeApiPoll call between them.

@oranagra oranagra changed the title fix unblock client latency improve unblock client latency Oct 5, 2021
@oranagra
Copy link
Member

oranagra commented Oct 5, 2021

I didn't see the same problem happen for a normal client sending XADD

@tzongw do you mean that the problem doesn't exist in that case? or just that that's not the case in which you found it?
i wanna simplify the description at the top (for the sake of release notes), and specify that it affects any unblocked client.

@tzongw
Copy link
Contributor Author

tzongw commented Oct 5, 2021

do you mean that the problem doesn't exist in that case? or just that that's not the case in which you found it?

@oranagra The problem doesn't exist for a normal client.

i wanna simplify the description at the top (for the sake of release notes), and specify that it affects any unblocked client.

Sure, fell free to modify the description.

@oranagra
Copy link
Member

oranagra commented Oct 6, 2021

ohh, now i see what you wrote about handleClientsBlockedOnKeys being called from processCommand (and the code comment which i didn't bother to read).
thanks.

@oranagra oranagra changed the title improve unblock client latency improve latency when a client is unblocked by module timer Oct 6, 2021
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Oct 6, 2021
@oranagra oranagra merged commit f5160ed into redis:unstable Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants