Skip to content

Add internal timer system for async timeout handling - #330

Open
bjosv wants to merge 8 commits into
valkey-io:mainfrom
bjosv:add-timer-system
Open

Add internal timer system for async timeout handling#330
bjosv wants to merge 8 commits into
valkey-io:mainfrom
bjosv:add-timer-system

Conversation

@bjosv

@bjosv bjosv commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Adds a pre-allocated timer pool that supports multiple concurrent async timers through a single adapter hook.
This allows adding additional timers (cluster topology refresh, c-ares DNS timeouts, health checks) without requiring adapter changes.

Timer framework (commit 1)

  • Pre-allocated pool of timers, sorted linked list by deadline
  • One timer processed per call (safe if callback frees the context)
  • Uses CLOCK_MONOTONIC / QueryPerformanceCounter

Async timeout migration (commit 2)

  • Separate connect_timer and command_timer with own callbacks
  • Connect timer cancelled on successful connect
  • Command timer tracks reply count to detect unresponsive servers
  • refreshTimeout is now a regular exported function (for TLS .so)
  • No adapter changes

Behavior change
Changing command_timeout at runtime no longer affects an already-running timer.
The new value takes effect after the current timer fires.

bjosv added 2 commits July 3, 2026 09:31
Add src/timer.c with a pre-allocated pool of timers managed as a
sorted linked list. Timers can be scheduled, cancelled, and
processed one at a time (to allow callbacks to free the context).

This is the foundation for running multiple concurrent timers
(command timeout, connect timeout, cluster topology refresh, etc.)
through a single adapter scheduleTimer hook.

Callers that need periodic behavior reschedule in their callback.

Uses CLOCK_MONOTONIC on Unix and QueryPerformanceCounter on Windows.

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Connect and command timeouts are now separate timers:
- connect_timer: scheduled during connect, cancelled on success
- command_timer: scheduled on first write when connected, tracks
  reply activity to detect unresponsive servers

valkeyAsyncHandleTimeout dispatches expired timers via
valkeyProcessTimers. No adapter changes required.

| Test                     | main   | PR     |
|--------------------------|--------|--------|
| 10000x PING pipelined    | 0.004s | 0.005s | (avg from 4 runs)
| 10000x INCRBY pipelined  | 0.004s | 0.004s | (avg from 4 runs)
| 10000x LRANGE pipelined  | 0.793s | 0.782s | (avg from 4 runs)

Note: changing command_timeout at runtime no longer affects an
already-running timer. The new value takes effect after the current
timer fires.

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv
bjosv requested a review from michael-grunder July 3, 2026 12:00
Comment thread src/timer.c Outdated
@michael-grunder

Copy link
Copy Markdown
Collaborator

This is a massive improvement.

I'm still going through it but I have a couple extra commits in a branch on my fork

Just the overflow comment and re-allowing a user to disable a timeout after it has been set. I think we still want that behavior?

Comment thread src/async.c
Comment thread src/async.c Outdated
bjosv and others added 6 commits July 13, 2026 11:30
Co-authored-by: Michael Grunder <michael.grunder@gmail.com>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Co-authored-by: Michael Grunder <michael.grunder@gmail.com>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Co-authored-by: Michael Grunder <michael.grunder@gmail.com>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Co-authored-by: Michael Grunder <michael.grunder@gmail.com>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv
bjosv force-pushed the add-timer-system branch from 4c393ca to 8236f01 Compare July 23, 2026 10:02
@bjosv

bjosv commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

@michael-grunder Moved the valkeyTimerList allocation now, alot better. I also added your improvement commits.

I wonder if its possible to get rid of refreshTimeout and its call in every _EL_ADD_READ/_EL_ADD_WRITE somehow now.
Previously it handled the multiplexing of timers, but now we might just be able to start each timer when connecting and when sending a command.

I think the command timeout could be moved already by starting the command_timer in valkeyAsyncAppendCmdLen instead.

The connect timer is harder since we need to start it in valkeyAsyncConnect*, or when the event-adapter is attached by the user via calls like valkeyPollAttach(). Maybe this was the reason for doing it in refreshTimeout from the start?

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.

2 participants