added thread lock to avoid conflicts between GC and parameter by rozyczko · Pull Request #175 · easyscience/core

rozyczko · 2025-12-13T10:41:09Z

Fixed the RuntimeError: dictionary changed size during iteration issue in the vertices method by implementing thread-safety: iterations handling modifications during garbage collection.
This seems to have been a concurrency problem with weak references, where the dictionary is being modified during iteration.

Take the attached script (.txt -> .py cause GitHub)
run_test_suite.txt

Run it in say, EasyReflectometryLib or corelib

This now runs fine.

Root Cause

The Map class uses weakref.WeakValueDictionary() to store object references. When:

generate_unique_name() is called (e.g., when creating a new Parameter)
It calls self.map.vertices() which returns list(self._store.keys())
During the iteration over .keys(), the garbage collector may run
GC cleans up objects with no remaining strong references
This removes entries from the WeakValueDictionary
Python raises RuntimeError: dictionary changed size during iteration

This is particularly likely to happen when:

Many temporary Parameter objects are created (e.g., in Lattice.volume property)
The GC runs during the iteration
Multiple objects are being created/destroyed in quick succession

creation/deletion

github-actions

This pull request does not contain a valid label. Please add one of the following labels: ['[scope] bug', '[scope] enhancement', '[scope] documentation', '[scope] significant', '[scope] maintenance']

codecov · 2025-12-13T13:20:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.15%. Comparing base (29e696e) to head (5b6a217).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #175      +/-   ##
===========================================
+ Coverage    80.80%   81.15%   +0.35%     
===========================================
  Files           52       52              
  Lines         4261     4267       +6     
  Branches       739      740       +1     
===========================================
+ Hits          3443     3463      +20     
+ Misses         633      624       -9     
+ Partials       185      180       -5

Flag	Coverage Δ
unittests	`81.15% <100.00%> (+0.35%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/easyscience/global_object/map.py	`96.92% <100.00%> (+7.50%)`	⬆️

unfounded fears of "overengineering"...

rozyczko · 2025-12-15T16:42:14Z

Nooope. Going back to the thread lock.

gc.disable() affects the GLOBAL garbage collector state across the whole process (all threads).
We use corelib in libraries (same thread) and in GUIs (in a separate - non-GUI thread).
Disabling GC in a GUI thread can have unintented consequences, the least being slowing down due to memory buildup.

This reverts commit 5d3ea2c.

damskii9992 · 2025-12-16T09:20:33Z

gc.disable() affects the GLOBAL garbage collector state across the whole process (all threads). We use corelib in libraries (same thread) and in GUIs (in a separate - non-GUI thread). Disabling GC in a GUI thread can have unintented consequences, the least being slowing down due to memory buildup.

Did you encounter issues in the application when you tried it out? It is only a (VERY) temporary disabling of GC, so memory buildup shouldn't occur.

rozyczko · 2025-12-16T09:57:18Z

gc.disable() affects the GLOBAL garbage collector state across the whole process (all threads). We use corelib in libraries (same thread) and in GUIs (in a separate - non-GUI thread). Disabling GC in a GUI thread can have unintented consequences, the least being slowing down due to memory buildup.

Did you encounter issues in the application when you tried it out? It is only a (VERY) temporary disabling of GC, so memory buildup shouldn't occur.

Doesn't matter. This is still a valid issue - we shouldn't do things in the library, which can globally affect behaviour of the code linked to it?

damskii9992 · 2025-12-16T09:56:14Z

src/easyscience/global_object/map.py

+        with self._lock:
+            if item_id in self._store.keys():
+                return self._store[item_id]
+            raise ValueError('Item not in map.')


We should be able to avoid this entire error if we just use normal introspection of the dictionary rather than first get the keys:

if item_id in self_store: return self._store[item_id]

Then we're not iterating over values and thus don't need this (it should also be substantially faster).

'''
• Key existence vs. value availability:
item_id in self._store checks only if the key is present. If the value has been collected, the key will also be removed, so you won’t get a dangling key.
• Iteration safety:
Iterating over self._store.keys() or items() is safe, but note that the contents may shrink during iteration if garbage collection occurs.
• Direct access:
self._store[item_id] will raise KeyError if the object was collected.
'''

So even with direct query we will get the same issue.

Actually you might be right.
Querying the dict directly with the KeyError except check should be enough. Let's check this with the test script

Wait, was that a ChatGPT prompt output? It obviously hallucinates. I literally said that its safe to iterate over self._store.keys(), which is what is causing our error.

Yes, this is what you get when you take AI verbatim. Some good points but all covered in nonsense.

damskii9992 · 2025-12-16T09:57:33Z

src/easyscience/global_object/map.py

+        Uses a threading lock to prevent RuntimeError from WeakValueDictionary.
+        """
+        with self._lock:
+            return vertex.unique_name in self._store.keys()


Again, we can avoid the lock and this issue if we don't call the keys() method. Just use regular inspection.

damskii9992 · 2025-12-16T09:58:26Z

src/easyscience/global_object/map.py

-        self.__type_dict[name].type = obj_type
+        with self._lock:
+            name = obj.unique_name
+            if name in self._store.keys():


damskii9992 · 2025-12-16T09:59:20Z

src/easyscience/global_object/map.py

            del self.__type_dict[vertex1][self.__type_dict[vertex1].index(vertex2)]

    def prune(self, key: str):
-        if key in self.__type_dict.keys():


And again here.

damskii9992 · 2025-12-16T10:01:12Z

src/easyscience/global_object/map.py

+            for vertex in self.vertices():
+                self.prune(vertex)
+            gc.collect()
+            self.__type_dict = {}


Actually, can't we get rid of this method and just use the built-in clear method of a regular dictionary?

damskii9992 · 2025-12-17T12:35:56Z

I really like the retry look implementation for the vertices method. If this works I'd say its' good to merge :)
Although it looks like codecov wants more tests.

added thread lock to avoid conflicts between GC and parameter

4835f1c

creation/deletion

rozyczko marked this pull request as ready for review December 13, 2025 10:41

github-actions bot requested changes Dec 13, 2025

View reviewed changes

rozyczko added [scope] bug Bug report or fix (major.minor.PATCH) [priority] highest Urgent. Needs attention ASAP labels Dec 13, 2025

github-actions bot approved these changes Dec 13, 2025

View reviewed changes

rozyczko assigned damskii9992 and Copilot Dec 13, 2025

Replace thread lock with explicit gc disable due to Christian's

5d3ea2c

unfounded fears of "overengineering"...

Revert "Replace thread lock with explicit gc disable due to Christian's"

4eae1d9

This reverts commit 5d3ea2c.

damskii9992 reviewed Dec 16, 2025

View reviewed changes

let's try querying self._store rather than its keys.

e226e5a

damskii9992 linked an issue Dec 17, 2025 that may be closed by this pull request

RuntimeError: dictionary changed size during iteration #100

Closed

rozyczko added 2 commits December 17, 2025 14:24

Merge branch 'develop' into dict_size_changed_fix

0e0623a

add unit tests to satisfy greedy codecov

5b6a217

rozyczko merged commit 6e823b2 into develop Dec 17, 2025
35 checks passed

rozyczko deleted the dict_size_changed_fix branch December 17, 2025 13:51

rozyczko mentioned this pull request Dec 17, 2025

Changed add_vertex behaviour on identical unique name #137

Closed

Conversation

rozyczko commented Dec 13, 2025

Root Cause

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rozyczko commented Dec 15, 2025

Uh oh!

damskii9992 commented Dec 16, 2025

Uh oh!

rozyczko commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozyczko Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damskii9992 commented Dec 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 13, 2025 •

edited

Loading

rozyczko Dec 16, 2025 •

edited

Loading