feat(core): Introduce client rate limiting (concurrent_request_limit)

[ceache]

Fixes #664
Relates to #458

Why is this needed?

During connection failovers, cluster leader elections, or when clients manage large watch hierarchies (such as with TreeCache), clients can generate an unbounded surge of get and get_children requests.

ZooKeeper servers enforce an incoming request queue limit (globalOutstandingLimit, typically 1,000 requests across all connected clients). When multiple clients or heavy recipes reconnect simultaneously, this sudden flood can saturate server receive queues, trigger request drops or timeouts, and induce cascading reconnect loops across the ensemble (thundering herd). Similarly, event-driven applications reacting to high-frequency external triggers can easily overwhelm the server if in-flight requests are not bounded.

Proposed Changes

• Opt-in concurrent_request_limit Configuration:

  • Added concurrent_request_limit: PositiveInt | None = None to KazooClient.__init__ (default is None, which disables rate limiting and preserves 100% backward compatibility).
  • Strongly typed using PositiveInt = Annotated[int, _Gt(0)] under if TYPE_CHECKING:.
  • Added runtime validation raising ConfigurationError("concurrent_request_limit must be greater than 0") if configured with a non-positive value.

• Wire-Level Throttling (Non-blocking & Deadlock-free):

  • Instead of blocking the calling thread or completion worker with a semaphore in _call() (which previously risked completion worker deadlocks on chained callbacks and double-release crashes on connection drops), rate limiting is implemented transparently at the transport layer in ConnectionHandler._connect_attempt.
  • The connection's select() loop checks len(client._pending) < client.concurrent_request_limit. When the in-flight wire limit is reached, _read_sock is excluded from select().
  • Incoming responses from the ZooKeeper server are prioritized. The instant a response packet arrives and is popped from _pending, capacity frees up and the next queued request is sent over TCP.

• Preserved Asynchronous Contract:

  • All asynchronous calls (get_async, create_async, etc.) return immediately with an IAsyncResult.
  • Completion worker callbacks and watch handlers can freely issue further async requests without deadlocking.

• Clean Drain on Connection Loss:

  • In the event of a disconnect, both _pending and _queue are cleanly notified and drained with ConnectionLoss without leaking state or causing double-release errors.

Related Issues & Discussions

• TreeCache recipe creating heavy load on ZK while reconnecting. #664: TreeCache recipe creating heavy load on ZK while reconnecting. — Directly addresses the reconnect surge by smoothing traffic bursts and preventing ensemble overload.
• Deadlock with TreeCache and reconnection #458: Deadlock with TreeCache and reconnection — Background context on request burst handling during cache reconnection.

Testing & Quality Assurance

• Added 7 dedicated unit tests in kazoo/tests/test_rate_control.py:
  1. Client initialization and argument validation (None, positive int, and ConfigurationError on non-positive values).
  2. Asynchronous calls never block the caller.
  3. Completion worker executing chained requests without deadlock.
  4. Connection loss drains both pending and queued requests cleanly.
  5. Select loop socket gating based on in-flight count.
  6. Pipeline flow simulation validating that in-flight requests never exceed the configured limit.
  7. TreeCache reconnect simulation (6 nodes, 12 concurrent operations) verifying deadlock-free completion under concurrent_request_limit=1.
• Clean static analysis:
  • mypy -p kazoo passes with 0 issues across 65 source files.
  • flake8 kazoo passes with 0 warnings/errors.

[ceache]

@python-zk/maintainers This is a first draft. Let me know if you like the direction and I'll add tests, etc.

Full disclosure, we have had a version of this patch in production for a while (with both gevent and threading handlers). I have not tested the eventlet handler directly.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.40%. Comparing base (4944f49) to head (185f210).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #683      +/-   ##
==========================================
+ Coverage   94.48%   95.40%   +0.92%     
==========================================
  Files          27       27              
  Lines        3810     3833      +23     
==========================================
+ Hits         3600     3657      +57     
+ Misses        210      176      -34     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[StephenSorriaux]

I think it is a great idea. Just wondering: from a user experience perspective, will there be any way to know the rate limit has been hit?

[ceache]

No, at least not in this current form. Sync requests will block, as if the zk server was slow. This was designed for async requests so that code, possibly in different threads, could simply queue them up knowing that the client would respect the set rate limit. Think of chains of get_children_async | get_async when walking a hierarchy. We could add logging, it would not be difficult, but I would not want it to be intrusive (i.e. debug?) I feel like throttling for rate limit is not an "issue", it is a feature. Does thay make sense?

…

On Sun, Nov 13, 2022, 12:53 Stephen Sorriaux ***@***.***> wrote: I think it is a great idea. Just wondering: from a user experience perspective, will there be any way to know the rate limit has been hit? — Reply to this email directly, view it on GitHub <#683 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIFTHQZ6D6LMQI7SJEFGBLWIETI5ANCNFSM6AAAAAAR6Q7REY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[StephenSorriaux]

Yes, that makes a lot of sense, thank you.

I agree with you, rate limit is a feature, but sometimes it can be hard to "understand"/be aware of it if there is no logs telling you "hey, rate limit is currently being triggered" or "hey, rate limit has been triggered N times in the past M seconds" (even if, I agree, there is already a info log at client startup). You're right this should not be intrusive, I know some projects display a "rate limit has been triggered" message once in a while (every 30s or so for instance), but I believe a debug message can work too if you think it is better/useful.

Writing this message, it makes me realize that this lib currently does not provide any metrics (if it would, a number of times rate limit has been triggered would be useful). Maybe it is something we could add to the backlog, but that is not the point of your current proposition that, again, I think is great.

[ceache]

I'll add a log message for now, and move to add some tests. About metrics, i was talking to opentelemetry folks at KubeCon about exactly that. it looks like it would be possible to use such a framework. I mean, they instrument SQLite3 and requests modules for example IMHO, of we can pull this off, this would be an ideal way to extract this kind of information, as well as request counts, error counts, connection length and a million other things i have always wanted to know about my zookeeper client but never knew how to ask 😜 On a more serious note, if you like the idea, i can create an issue for this and try to gather more Intel?

…

On Tue, Nov 15, 2022, 18:41 Stephen Sorriaux ***@***.***> wrote: Yes, that makes a lot of sense, thank you. I agree with you, rate limit is a feature, but sometimes it can be hard to "understand"/be aware of it if there is no logs telling you "hey, rate limit is currently being triggered" or "hey, rate limit has been triggered N times in the past M seconds" (even if, I agree, there is already a info log at client startup). You're right this should not be intrusive, I know some projects display a "rate limit has been triggered" message once in a while (every 30s or so for instance), but I believe a debug message can work too if you think it is better/useful. Writing this message, it makes me realize that this lib currently does not provide any metrics (if it would, a number of times rate limit has been triggered would be useful). Maybe it is something we could add to the backlog, but that is not the point of your current proposition that, again, I think is great. — Reply to this email directly, view it on GitHub <#683 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIFTHTGT7CYMUWYAPK2NWLWIQNS3ANCNFSM6AAAAAAR6Q7REY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[StephenSorriaux]

I'll add a log message for now, and move to add some tests. About metrics, i was talking to opentelemetry folks at KubeCon about exactly that. it looks like it would be possible to use such a framework. I mean, they instrument SQLite3 and requests modules for example IMHO, of we can pull this off, this would be an ideal way to extract this kind of information, as well as request counts, error counts, connection length and a million other things i have always wanted to know about my zookeeper client but never knew how to ask stuck_out_tongue_winking_eye On a more serious note, if you like the idea, i can create an issue for this and try to gather more Intel?

Sorry for my late reply @ceache, busy weeks :(

To be honest, I really like the idea and it would be great if you can get some intel about it, especially on how we should make those metrics available to our users: should we provide some interface so that they can plug whatever client they want? Should we actually provide some integration ourselves (like prometheus, etc.)? I totally feel you on this subject, I also love to get tons of metrics about what's in production!

[Buffer0x7cd]

Hey Folks, any update on the progress of this PR ? Let me know if there is any help needed to complete this feature.

[ceache]

@python-zk/maintainers I think I have implemented all what we discussed. All the more "future looking" ideas discussed here (prometheus, OTL,) would be addressed in future PRs

[StephenSorriaux]

can a test be added to check that we get blocked and then get released?