ADR Suggestion Removal of weakref dicts (2)

[rozyczko]

Continuation of #218 with Session object used for hard ref tracking.

Session-Based Object Registry Replacing WeakRef GlobalObject

Elimination of race conditions caused by iterating a WeakValueDictionary inside a global singleton (GlobalObject + Map).

---

Context and Problem Statement

Every EasyScience object (NewBase and its descendants) registered itself at construction time in a process-wide singleton:

# old new_base.py
self._global_object = global_object
self._global_object.map.add_vertex(self, obj_type='created')

Map._store was a WeakValueDictionary. Objects were pruned automatically by the garbage collector via weakref.finalize callbacks.

This design produced three types of bugs:

1. Concurrent-modification crashes – RuntimeError: dictionary changed size during iteration occurred when GC ran while other code iterated _store. The workaround (while True: try: list(...) except: continue) bounded neither latency nor correctness.

2. Global mutable state – a single shared registry made isolated testing impossible without calling global_object._clear() between every test, and prevented two independent object graphs from coexisting in the same process.

3. Entangled concerns – naming, graph tracking, undo/redo history, and script recording were all funnelled through one singleton, making each concern hard to evolve independently.

The question addressed here is: what should replace the GlobalObject + WeakValueDictionary combination, and why is Session the right anwer?

Considered Options

Option A – Class-level counter on NewBase (no separate class)

As described in #218

class NewBase:
    _name_counters: ClassVar[dict[str, int]] = {}
    _registry: ClassVar[dict[str, NewBase]] = {}

All state lives on the class itself.

Option B – Module-level plain dict + functions

Also proposed in #218

_registry: dict[str, NewBase] = {}

def reserve_name(name, obj): ...
def release_name(name): ...

State lives in module globals, accessed through free functions.

Option C – Session as a dedicated mutable-state holder - PROPOSED HERE.

class Session:
    def __init__(self) -> None:
        self._lock = threading.RLock()
        self._registry: dict[str, NewBase] = {}
        self._name_counters: dict[str, int] = {}
        self._children: dict[str, list[str]] = {}
        self._parent: dict[str, str | None] = {}

State is an instance. A module-level default session provides the single-global behaviour of the old code. Callers that need isolation create their own Session and pass it via the constructor.

---

Proposed solutuion: Option C

The key insight is that the problematic property of the old design was not global state per se. It was uncontrollable global state.

Why Session must be a separate class, not state on NewBase

This is the main point of the design.

1. The state is shared across instances, not per-instance

A registry maps names -> objects. Name counters track how many objects of each class prefix exist. Parent/child edges connect pairs of objects. None of this state belongs to any single object - it is relational state that spans many objects simultaneously. Attaching it to NewBase instances would require every setter and method that mutates the registry to reach across to class-level or module-level storage anyway, producing the same coupling as the old GlobalObject.

3. The lock must co-locate with the data it protects

threading.RLock is only meaningful when the lock and the data it guards move together. If the registry lived in a class attribute, every test that resets the class attribute would also have to reset the lock — creating a TOCTOU window. Because Session.__init__ creates both the lock and the empty dicts in one atomic step, the invariant "lock and data are always in sync" is structurally enforced.

4. Lifecycle is explicit and testable

A Session can be created, used, and discarded. Disposal of all objects inside it is well-defined. With class-level or module-level dicts, "clearing the registry" means mutating shared state in place, which is still a global side effect observable by any concurrent code. Creating a fresh Session() creates a completely new namespace.

5. Future extensibility without breaking NewBase

Serialising an entire session, sharing sessions across processes, attaching per-session event hooks - all these potential future features have a natural home on Session without touching NewBase's public API. With class-level state there is nowhere clean to attach such behaviour.

---

Positive Consequences

• Race condition eliminated structurally: WeakValueDictionary and weakref.finalize are gone.
• Tests use set_default_session(Session()) in an autouse fixture. No inter-test pollution possible.
• Two independent models with overlapping parameter names can coexist in the same process.
• Thread safety is explicit: all write paths in Session hold self._lock; the guarantee is documented and auditable.
• NewBase constructor signature gains session=None, which is backward-compatible: all existing call sites continue to work and receive the default session transparently.

Negative Consequences / Known Limitations

• dispose() is now the caller's responsibility. The old design used GC finalizers for cleanup; the new design requires explicit obj.dispose(). Forgetting to call it leaks entries in the session registry (strong references, so no GC recovery). This is a deliberate trade-off: predictable leaks are easier to debug than unpredictable GC races.
• Undo/redo and script recording are not yet migrated. The @property_stack decorator has been removed from display_name.setter in this change. Those features will be addressed in a follow-up ADR once a Session-scoped undo stack design is agreed.

---

[rozyczko]

The main problem is the fact that Session holds strong references.

b1 = NewBase()  # Creates 'NewBase_0'
b2 = NewBase()  # Creates 'NewBase_1'  
b1 = b2         # Original b1 object becomes unreachable
gc.collect()    # Does NOT free the object
session.all_names()     # Still shows ['NewBase_0', 'NewBase_1']

This causes memory leaks and dangling refs.
The only solution I can find is to be diligent and dispose of every element from the session when necessary with

dispose()

that is,

b1 = NewBase()  # Creates 'NewBase_0'
b2 = NewBase()  # Creates 'NewBase_1'
b1.dispose()
b1 = b2         # Original b1 object becomes unreachable
session.all_names()     # shows now ['NewBase_1'] as it should

This is clearly unacceptable.

WE HAVE TO USE WEAKREFs