fix: reap stale netius HTTP clients from dead threads#85
Conversation
When WSGI worker threads die and get recycled, old thread IDs with their HTTPClient objects are never cleaned up from the global _netius_clients dict, causing unbounded memory growth. This adds a periodic reaper that runs when creating a new client, cleaning up entries for dead threads. Also fixes a TOCTOU race on client insertion by moving it inside the lock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d92f7ffa8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ The license for the module """ | ||
|
|
||
| import unittest | ||
| import unittest.mock |
There was a problem hiding this comment.
Importing unittest.mock unconditionally breaks this test module on Python 2.7, where unittest.mock is not available; src/appier/test/http.py will fail at import time before any tests execute in a Python-2 environment. Since the project targets Python 2.7+ compatibility, this should follow the guarded/fallback pattern already used in other tests (for example, src/appier/test/config.py).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses unbounded growth of per-thread Netius HTTPClient instances in multi-threaded environments by introducing a stale-client reaper and tightening thread-safety around client registration.
Changes:
- Add
_reap_netius_clients()to remove andcleanup()clients associated with dead threads. - Invoke reaping lazily from
_client_netius()before creating a new client. - Move
_netius_clients[tid]insertion underACCESS_LOCKto avoid a TOCTOU race.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/appier/http.py |
Adds stale-client reaping and locks client registration to prevent leaks/races. |
src/appier/test/http.py |
Adds unit tests covering stale-client reaping and cleanup behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not _netius_clients: | ||
| return | ||
| live_tids = set(t.ident for t in threading.enumerate()) | ||
| ACCESS_LOCK.acquire() | ||
| try: |
There was a problem hiding this comment.
live_tids is captured before acquiring ACCESS_LOCK, so another thread can create/insert its client into _netius_clients after the snapshot but before the sweep. That thread’s TID won’t be in live_tids, causing its (live) client to be incorrectly reaped/cleaned up. Compute live_tids while holding ACCESS_LOCK (and ideally also move the empty-check under the lock) so the snapshot + sweep are atomic w.r.t. insertions.
| if not _netius_clients: | |
| return | |
| live_tids = set(t.ident for t in threading.enumerate()) | |
| ACCESS_LOCK.acquire() | |
| try: | |
| ACCESS_LOCK.acquire() | |
| try: | |
| if not _netius_clients: | |
| return | |
| live_tids = set(t.ident for t in threading.enumerate()) |
| dead_tids = [tid for tid in _netius_clients if tid not in live_tids] | ||
| for tid in dead_tids: | ||
| client = _netius_clients.pop(tid, None) | ||
| if not client: |
There was a problem hiding this comment.
if not client: can skip cleanup for valid client objects that define falsey truthiness. Since pop(..., None) is used, this should check explicitly for None (e.g., if client is None:) to avoid leaving an uncleaned client removed from the map.
| if not client: | |
| if client is None: |
| try: | ||
| client.cleanup() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The except Exception: pass around client.cleanup() suppresses all cleanup failures silently, making resource leaks/debugging much harder in production. Consider at least logging the exception (possibly at debug level) before continuing, or narrowing the caught exception types if there are known benign failures.
| pass | |
| logging.getLogger(__name__).debug( | |
| "Failed to cleanup netius client for dead thread %s", | |
| tid, | |
| exc_info=True, | |
| ) |
| alive_tid = threading.current_thread().ident | ||
| dead_tid = 99999999 | ||
|
|
There was a problem hiding this comment.
Using a hard-coded dead_tid = 99999999 can collide with a real thread ident on some platforms/runtimes, which would make this test flaky (it would treat the entry as “alive”). Prefer deriving a guaranteed-dead TID at runtime (e.g., compute a value not in {t.ident for t in threading.enumerate()}) so the test is deterministic.
| dead_client = unittest.mock.MagicMock() | ||
| dead_tid = 99999999 | ||
|
|
There was a problem hiding this comment.
Same issue here: dead_tid = 99999999 is not guaranteed to be absent from threading.enumerate() idents, so the test can become flaky on some systems. Compute a TID that is definitely not in the current live set instead of relying on a magic constant.
|
Closing this as won't fix - this does not make sense for the time being |
Summary
_reap_netius_clients()to clean up HTTPClient objects from dead worker threads in the global_netius_clientsdict, preventing unbounded memory growth in long-running multi-threaded WSGI applicationsTest plan
test_reap_netius_clients- verifies dead thread entries are removed andcleanup()called, while alive thread entries are preservedtest_client_netius_cleanup- verifies all dead entries are fully reaped from an empty-after-reap scenario🤖 Generated with Claude Code