Repository navigation
Conversation
Mixed activityActivity patterns show a mix of organic and automated signals. Evidence
Last 5 PRs:
This is an automated analysis by AgentScan |
d6337a6 to
1df9482
Compare
pyba-chronographer
Bot
added
the
bot:chronographer:provided
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #859 +/- ##
==========================================
+ Coverage 78.20% 78.69% +0.48%
==========================================
Files 41 41
Lines 4791 4835 +44
Branches 548 554 +6
==========================================
+ Hits 3747 3805 +58
+ Misses 905 891 -14
Partials 139 139 |
|
Hey, just dropping a quick note here since I just posted this and #858 with Claude, as I want to be clear that while Claude wrote most/all of the code here, there is a real human behind these driving and doing first-pass reviews, and these are bugs I've been tracking internally for quite a while. We use cheroot to serve parts of an in-development RPC system in OpenStack's oslo.messaging project, and we trip over this periodically depending on deployment style. I will be tracking these and am happy to make adjustments based on feedback, or drop these PRs if there's a better approach from someone who knows the codebase better than I (and AI) do. Thanks! |
|
I'm pretty sure the failed test is not related to this, maybe someone can re-run it? |
|
Hi, thanks for the PR. Can't promise timely reviews but maybe @julianz- will be able to spare a minute. Feel free to drop by our matrix. I'm sometimes more responsive there. I've restarted the job, it seems like it hit GHA-level timeout while also having been marked as green. It might be wise to tag people who looked at this and some surrounding modules last, just in case. cc @pR0Ps @itamarst |
|
Thanks, no worries. I have some workarounds on my side in my oslo.messaging patches, but obviously that's not ideal long term, and I figured someone else may have run into this. |
julianz-
left a comment
julianz-
left a comment
There was a problem hiding this comment.
Overall logic seems fine but I have reworded various descriptions to make them easier to understand.
|
I pushed one more commit, fe81ecd, which only adds a test: it stops the |
Define _wait_until() after the module's markers instead of among the statistics tests. The thread pool tests of cherrypy#859 need the same helper and add it, byte for byte, at the same place, so the two changes merge into a single definition whichever lands first. Assisted-By: Anthropic Claude Opus 5.5 Signed-off-by: Adam Harwell <flux.adam@gmail.com>
|
One more small test-only commit, b25eaf2: the thread pool tests now use a general |
Define _wait_until() after the module's markers instead of among the statistics tests. The thread pool tests of cherrypy#859 need the same helper and add it, byte for byte, at the same place, so the two changes merge into a single definition whichever lands first. Assisted-By: Anthropic Claude Opus 5.5 Signed-off-by: Adam Harwell <flux.adam@gmail.com>
b25eaf2 to
d641734
Compare
shrink() subtracted the shutdowns that earlier calls had queued from the amount it was asked for, before it removed the threads that had already exited. So a call made before an earlier call's threads were reaped was mostly ignored: four shrink(20) calls in a row retired 20 of 90 threads, and 40 when made a second apart. The idle property already leaves those threads out, so a caller that sizes the amount from it had them subtracted twice. The threads above the minimum were also counted with those already shutting down: shrink(40), then shrink(80), on 50 threads with a minimum of 10 retired all 50, and left 30 requests queued. shrink() now asks up to that many more threads to shut down, among those above the minimum that are not shutting down yet. stop() queued a request for every thread in its list, including the threads that shrink() had already asked to shut down, so requests were left in the queue. The next start() lost its new threads to them and waited for those threads forever. stop() also kept the pending shutdowns counted, so a restarted pool reported none of its threads idle, and its next stop() queued too few requests and waited for its threads forever. stop() now queues a request only for each thread that shrink() has not asked already, and forgets the pending shutdowns once it has joined the threads. The tests use a general _wait_until() helper. The statistics tests of cherrypy#858 add the same helper, byte for byte, at the same place, so the two changes merge into a single definition whichever lands first. Assisted-By: Anthropic Claude Opus 5.5 Assisted-By: OpenAI Codex (GPT-6) Signed-off-by: Adam Harwell <flux.adam@gmail.com>
Every HTTPServer registered its statistics in the process-wide logging.statistics when it was constructed, and nothing removed them. Their callbacks hold the server, so every server ever built stayed alive with its gateway: stopped ones, ones whose prepare() failed to bind, and ones that never started. A server is now registered only while it serves, from prepare() to stop(), and clear_stats() replaces its entry while it serves. Every worker thread also left its entry in its server's Worker Threads statistics when it exited, so a pool that grows and shrinks again kept every thread it ever started. Each worker now registers with a small collector that clear_stats() creates. On exit, the worker adds its final counters to the collector's retired totals and removes its record, so the server totals still count its work: requests, bytes read and written, work time, and both throughputs, which remain the sum of each worker's rate. One lock covers registering, retiring and reading the totals, so a retiring worker is counted exactly once. A worker retires into the collector it joined, so one that outlives clear_stats() cannot bring the cleared counters back. A server whose own clear_stats() creates no collector keeps no worker records, rather than failing to start. The tests define a general _wait_until() helper after the module's markers. The thread pool tests of cherrypy#859 add the same helper, byte for byte, at the same place, so the two changes merge into a single definition whichever lands first. Assisted-By: Anthropic Claude Opus 5.5 Assisted-By: OpenAI Codex (GPT-6) Signed-off-by: Adam Harwell <flux.adam@gmail.com>
d641734 to
4408b02
Compare
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
I'm not quite comfortable with this part of the code base so I'm going to need more people to give opinions. And maybe run some agentic review against this too.
Maybe @julianz- / @jaraco / @the-allanc / @cyraxjoe could spare a minute.
| ) | ||
|
|
||
|
|
||
| def _wait_until(condition, timeout=10): |
There was a problem hiding this comment.
We should start using kwarg-only params in new code for readability (start the param def with an asterisk). Also, I believe, this might be called a predicate, eh?
Although, it's not obvious that it's callable from the name so we should probably use a verb like compute_condition so when it's called later, it doesn't look that weird.
Also, I recommend adding type annotations for all newly added code).
|
|
||
| def shrink(self, amount): | ||
| """Kill off worker threads (not below self.min).""" | ||
| # Grow/shrink the pool if necessary. |
There was a problem hiding this comment.
Have you looked into git diff (maybe over in cherrypy/cherrypy where the original Git tree came from) to see why the comment said "grow"? I just want to make sure that by removing the comment we don't lose any breadcrumbs.
There was a problem hiding this comment.
Looks like this predates @the-allanc's refactoring introduced in #204 — I'm somewhat scared to touch that self._pending_shutdowns that self._clear_dead_threads() mutates.
But the comment itself seems to have existed even before the module move (c9f9d72).
There was a problem hiding this comment.
The comment first appeared in cherrypy/cherrypy@d070ca0, back when nobody knew how to deal with maintaining a dual python2+python3 codebase, right before six was invented.
|
@julianz- this needs a more in-depth review now with full approval if all's good. |
There was a problem hiding this comment.
🟡 Changes recommended
Shutdown accounting during concurrent or interrupted shrink() can queue too few shutdown requests, causing stop() to hang indefinitely despite its timeout.
2 open issues
What changed in this PR
This PR addresses #861 by updating Cheroot’s thread-pool shutdown accounting to preserve the minimum pool size and support restarting after shrinking and stopping.
Changes:
- Make repeated
shrink()calls request additional retirements without double-counting pending shutdowns. - Adjust
stop()to account for existing shutdown requests and clear pending state afterward. - Add four regression cases covering repeated shrinking, minimum-size preservation, and restarts.
| File | Description |
|---|---|
docs/changelog-fragments.d/859.bugfix.rst |
Document shutdown-accounting fixes and restart behavior. |
cheroot/workers/threadpool.py |
Update shrink limits, shutdown-request counts, and pending-state cleanup. |
cheroot/test/test_server.py |
Add fixtures, waiting helpers, and thread-pool regression tests. |
Review effort: Max
| # Some threads may already have a shutdown request from an earlier | ||
| # shrink() call. Add a request for every thread that does not have | ||
| # one yet. | ||
| for _ in range(len(self._threads) - len(self._pending_shutdowns)): |
| # Some threads may already have a shutdown request from an earlier | ||
| # shrink() call. Add a request for every thread that does not have | ||
| # one yet. | ||
| for _ in range(len(self._threads) - len(self._pending_shutdowns)): |
What kind of change does this PR introduce?
What do these changes do?
shrink()subtracted the shutdowns that earlier calls had queued from the amount it was asked for, before it removed the threads that had already exited. So a call made before an earlier call's threads were reaped was mostly ignored.idlealready leaves those threads out, so a caller that sizes the amount from it, as dynpool does, had them subtracted twice. The threads above the minimum were also counted together with those already shutting down, so the pool could drop belowmin.stop()queued a shutdown request for every thread it listed, including those thatshrink()had already retired but that were not reaped yet, and never reset_pending_shutdowns. The requests left over then stopped the threads of the nextstart(), and itsgrow()waited for them forever.HTTPServer.prepare()on a stopped server starts the pool again, as CherryPy does when its bus restarts.shrink()now asks up to that many more threads to shut down, among those above the minimum that are not shutting down yet.stop()queues one request for each live thread thatshrink()has not asked already, and forgets the pending shutdowns once the threads are joined.shrink(20)on 90 threads,min10shrink(40), thenshrink(80), on 50 threads,min10start()aftershrink()andstop()The four new tests fail without the change, including one that stops the pool while a shrink is still pending; the restart test fails by a timeout rather than hanging the run.
Are there changes in behavior for the user?
shrink(amount)now retires up toamountmore threads, counting those already shutting down as gone, asidledoes, and never belowmin. #204 settled thatamountcounts the shutdowns already scheduled. That still holds for a caller that sizes it fromidle, but one that subtracts pending shutdowns itself would now shrink more. Nothing in cheroot or CherryPy callsshrink().Is it a substantial burden for the maintainers to support this?
No. It changes how two methods count, in a few lines, and the new tests cover all three cases.
Related issue number
Fixes #861. A related statistics retention problem, #860, is fixed in #858.
Checklist
Tip
New to open source contributions? Making a lovely PR is a great read.
and description in grammatically correct, complete sentences
docs/) and inline docstrings reflect the changes(see good commit messages)
(can be done once the PR number is known; alternatively, you can
use a related issue number if one exists)
code of conduct
This change, its tests and this description were written by an AI agent (Anthropic Claude Opus 5.5) on behalf of Adam Harwell (@rm-you). The tests were run locally with Python 3.11 (217 passed, 3 existing expected failures), and with #858 merged on Python 3.11, 3.12 and 3.13; all pre-commit hooks passed on all files, including
mypy-stubtestwith Python 3.12.🤖 Generated with Claude Code