Skip to content

[BUG] Fix thread-unsafe singleton initialization in factory functions#82

Open
direkkakkar319-ops wants to merge 5 commits into
sktime:mainfrom
direkkakkar319-ops:SingletonPattern-DirekKakkar
Open

[BUG] Fix thread-unsafe singleton initialization in factory functions#82
direkkakkar319-ops wants to merge 5 commits into
sktime:mainfrom
direkkakkar319-ops:SingletonPattern-DirekKakkar

Conversation

@direkkakkar319-ops
Copy link
Copy Markdown

What does this implement/fix? Explain your changes.

All five singleton factory functions (get_registry, get_handle_manager, get_executor, get_tag_resolver, get_composition_validator) used a simple if None check with no locking, making them unsafe under concurrent access. When fit_predict_async offloads work to thread pool executors via loop.run_in_executor(), multiple threads can race through the if _instance is None check simultaneously, creating duplicate instances and leading to inconsistent state (e.g., duplicate registry loads, split handle stores).
This PR applies the double-checked locking pattern to all five singletons using threading.Lock

if _instance is None:          # Fast path
    with _lock:                # Serialize only during first initialization
        if _instance is None:  # Re-check under lock
            _instance = Class()
  • Each file receives two minimal changes:
  1. import threading added to imports
  2. A module-level threading.Lock() and the double-checked locking pattern in the factory function

Does your contribution introduce a new dependency? If yes, which one?

No. threading is part of the Python standard library.

What should a reviewer concentrate their feedback on?

  1. Correctness of the double-checked locking pattern across all five files
  2. Whether any of these singletons should additionally protect their internal mutable state with locks for thread-safe mutation, beyond just safe initialization .

Any other comments?

Double-checked locking was used instead of functools.lru_cache(maxsize=1) because it keeps the existing global pattern (so reset/tests stay simple), avoids cache-related overhead and GC complications, and ensures the fast path has no lock overhead—just a quick None check.

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
For new estimators

None

pytest pass locally
image

lint pass locally
image

@direkkakkar319-ops
Copy link
Copy Markdown
Author

Hey! @Shashankss1205 , This PR fixes a race condition in all five singleton factory functions. Since fit_predict_async uses loop.run_in_executor() to offload work to threads, the existing if None checks could be hit concurrently, creating duplicate instances.

@direkkakkar319-ops direkkakkar319-ops marked this pull request as ready for review April 5, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants