ModelEngine-Group / ModelEngine-Group/nexent
GlobalThreadPool singleton: unsynchronised `__new__` and silently ignored `max_workers`
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 5.9k
- Forks
- 731
- Avg merge
- 19h 34m
- Merged PRs (30d)
- 172
Description
backend/utils/thread_utils.py defines a process-wide thread pool used to fan out async work:
class GlobalThreadPool:
_instance = None
def __new__(cls, max_workers=10):
if cls._instance is None:
cls._instance = super().__new__(cls)
cls._instance.pool = ThreadPoolExecutor(max_workers=max_workers)
atexit.register(cls._instance.pool.shutdown)
return cls._instance
Two distinct problems here:
1. The singleton is not thread-safe. Two threads racing into GlobalThreadPool(...) before _instance is set can both pass the if cls._instance is None check, each construct a ThreadPoolExecutor, and one of them silently overwrites the other. The discarded executor leaks until shutdown (and its atexit shutdown still fires against a dangling reference). Compare with AgentRunManager in backend/agents/agent_run_manager.py:13-21 which correctly uses a class-level lock.
2. max_workers is silently ignored after first construction. Anywhere in the codebase, GlobalThreadPool(max_workers=20) returns the previously-constructed instance with the old size. Today line 19 creates the singleton with max_workers=5, but the default argument is max_workers=10 — so even reading the file gives no obvious answer to "what is the pool size?". Any caller passing a different value will be confused when it has no effect.
Suggested fix
import threading
class GlobalThreadPool:
_instance = None
_lock = threading.Lock()
def __new__(cls, max_workers=10):
if cls._instance is None:
with cls._lock:
if cls._instance is None:
instance = super().__new__(cls)
instance.pool = ThreadPoolExecutor(max_workers=max_workers)
atexit.register(instance.pool.shutdown)
cls._instance = instance
elif max_workers != cls._instance.pool._max_workers:
logger.warning(
"GlobalThreadPool already initialised with max_workers=%d; "
"requested %d will be ignored",
cls._instance.pool._max_workers, max_workers,
)
return cls._instance
Or — cleaner — drop the singleton-via-__new__ pattern in favour of a module-level instance constructed once at import time.
Severity: Medium (resource leak + footgun, not a correctness bug for the current single import site).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with backend/utils/thread_utils.py and inspect the GlobalThreadPool construction and its current call at line 19. Compare the initialization approach with the class-level lock in backend/agents/agent_run_manager.py:13-21, then check callers for differing max_workers values. Done means concurrent initialization no longer creates a discarded executor and the handling of later max_workers requests is explicit and covered by relevant tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100