Skip to content

Reordered operations in Map.add_vertex#197

Merged
rozyczko merged 1 commit intomore_weakref_fixesfrom
finalizer_race_condition_fix
Feb 11, 2026
Merged

Reordered operations in Map.add_vertex#197
rozyczko merged 1 commit intomore_weakref_fixesfrom
finalizer_race_condition_fix

Conversation

@seventil
Copy link

Fix is made to avoid hitting a race condition due to weakref.finalize() being called. It seems that .finalize temporarily deletes references of the object passed to it from dicts, therefore trying to get it from __type_dict or _store might randomly raise a KeyError.

This PR fixes failing tests from easyscience/dynamics-lib#96

Fix is made to avoid hitting a race condition due to weakref.finalize() being called. It seems that .finalize temporarily deletes references of the object passed to it from dicts, therefore trying to get it from __type_dict or _store might randomly raise a KeyError.
@seventil seventil self-assigned this Feb 10, 2026
@seventil seventil added [scope] bug Bug report or fix (major.minor.PATCH) [area] global_object Anything related to the global_object [priority] medium Normal/default priority labels Feb 10, 2026
@codecov
Copy link

codecov bot commented Feb 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.16%. Comparing base (bd10653) to head (787222d).
⚠️ Report is 5 commits behind head on more_weakref_fixes.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##           more_weakref_fixes     #197   +/-   ##
===================================================
  Coverage               81.15%   81.16%           
===================================================
  Files                      52       52           
  Lines                    4267     4268    +1     
  Branches                  740      740           
===================================================
+ Hits                     3463     3464    +1     
  Misses                    624      624           
  Partials                  180      180           
Flag Coverage Δ
unittests 81.16% <100.00%> (+<0.01%) ⬆️

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.93% <100.00%> (+0.01%) ⬆️

@rozyczko
Copy link
Member

How is this fixing the issue? Can you explain what those lines are changing?

@seventil seventil changed the base branch from develop to more_weakref_fixes February 10, 2026 10:27
@seventil
Copy link
Author

How is this fixing the issue? Can you explain what those lines are changing?

Just for reference, the traceback that lead to the creation of this PR

        self.__type_dict[name] = _EntryList()  # Add objects type to the list of types
>       self.__type_dict[name].finalizer = weakref.finalize(self._store[name], self.prune, name)
        ^^^^^^^^^^^^^^^^^^^^^^
E       KeyError: 'Parameter_338'

Weird thing is that here we see sequential __setitem__ call and __getitem__ call on the same dict resulting in KeyError, which just doesn't make any sense.

Here is my assumption of what is happening:
when wekref.finalize() is being called with some object passed as an argument, the dicts that hold a reference to such object are being renewed -> key-val pairs dissapear and then return back to the dicts. In some cases, trying to get an item from a dict while the dicts are being 'renewed' results in a KeyError. This is just my observation from try runs, I don't know how finalize actually works underneath the python API.

Basically, the line self.__type_dict[name].finalizer = weakref.finalize(self._store[name], self.prune, name) makes unnecesary calls to self.__type_dict.__getitem__ and to self._store.__getitem__.
If removing unnecessary calls also happens to solve the issue with "disappearing" keys, I think it's a worthwile change.
It doesn't change the logic of the method, but rather just semantics, so why not try?

I tested this change in EasyDynamics, which fixed the failing tests from easyscience/dynamics-lib#96

I didn't investigate why tests fail in this branch, but I expect that more_weakref_fixes hold the fixes for the issue I have here.

@seventil
Copy link
Author

@rozyczko could you please merge this into your branch if it's ok?

@rozyczko rozyczko merged commit 03631c2 into more_weakref_fixes Feb 11, 2026
31 of 38 checks passed
@rozyczko rozyczko deleted the finalizer_race_condition_fix branch February 11, 2026 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[area] global_object Anything related to the global_object [priority] medium Normal/default priority [scope] bug Bug report or fix (major.minor.PATCH)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants