Skip to content

fix: reap stale netius HTTP clients from dead threads #85

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.

Sign up for GitHub

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

Closed
joamag wants to merge 1 commit into from
Closed

fix: reap stale netius HTTP clients from dead threads #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions src/appier/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,10 +908,20 @@ def _client_netius(level=logging.CRITICAL):
if netius_client:
return netius_client

# reaps stale clients from dead threads before creating
# a new one, this prevents unbounded growth of the global
# clients map in long-running multi-threaded applications
_reap_netius_clients()

# creates the "new" HTTP client for the current thread and registers
# it under the netius client structure so that it may be re-used
# it under the netius client structure so that it may be re-used, the
# insertion is done inside the lock to avoid TOCTOU race conditions
netius_client = netius.clients.HTTPClient(auto_release=False)
_netius_clients[tid] = netius_client
ACCESS_LOCK.acquire()
try:
_netius_clients[tid] = netius_client
finally:
ACCESS_LOCK.release()

# in case this is the first registration of the dictionary a new on
# exit callback is registered to cleanup the netius infra-structure
Expand Down Expand Up @@ -983,6 +993,26 @@ def _callback(protocol, parser, message):
return extra


def _reap_netius_clients():
global _netius_clients
if not _netius_clients:
return
live_tids = set(t.ident for t in threading.enumerate())
ACCESS_LOCK.acquire()
try:
Comment on lines +998 to +1002

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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())

Copilot uses AI. Check for mistakes.
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:

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if not client:
if client is None:

Copilot uses AI. Check for mistakes.
continue
try:
client.cleanup()
except Exception:
pass

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
pass
logging.getLogger(__name__).debug(
"Failed to cleanup netius client for dead thread %s",
tid,
exc_info=True,
)

Copilot uses AI. Check for mistakes.
finally:
ACCESS_LOCK.release()


def _cleanup_netius():
global _netius_clients
for netius_client in _netius_clients.values():
Expand Down
48 changes: 48 additions & 0 deletions src/appier/test/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
""" The license for the module """

import unittest
import unittest.mock

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore Python 2-compatible mock import

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 👍 / 👎.

import threading

import appier
Expand Down Expand Up @@ -262,3 +263,50 @@ def test_invalid(self):
self.assertRaises(
BaseException, lambda: appier.get("https://invalidlargedomain.org/")
)

def test_reap_netius_clients(self):
dead_client = unittest.mock.MagicMock()
alive_client = unittest.mock.MagicMock()

alive_tid = threading.current_thread().ident
dead_tid = 99999999

Comment on lines +271 to +273

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
original = getattr(appier.http, "_netius_clients", None)
try:
appier.http._netius_clients = {
dead_tid: dead_client,
alive_tid: alive_client,
}

appier.http._reap_netius_clients()

self.assertEqual(dead_tid in appier.http._netius_clients, False)
self.assertEqual(alive_tid in appier.http._netius_clients, True)
dead_client.cleanup.assert_called_once()
alive_client.cleanup.assert_not_called()
finally:
if original is None:
if hasattr(appier.http, "_netius_clients"):
del appier.http._netius_clients
else:
appier.http._netius_clients = original

def test_client_netius_cleanup(self):
dead_client = unittest.mock.MagicMock()
dead_tid = 99999999

Comment on lines +295 to +297

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
original = getattr(appier.http, "_netius_clients", None)
try:
appier.http._netius_clients = {dead_tid: dead_client}

appier.http._reap_netius_clients()

self.assertEqual(dead_tid in appier.http._netius_clients, False)
self.assertEqual(len(appier.http._netius_clients), 0)
dead_client.cleanup.assert_called_once()
finally:
if original is None:
if hasattr(appier.http, "_netius_clients"):
del appier.http._netius_clients
else:
appier.http._netius_clients = original
Loading