Skip to content

Conversation

@iisakkirotko
Copy link
Contributor

Description

While implementing viewer closing in glue-jupyter, I found that ListCallbackProperty and DictCallbackProperty would often have a dangling references to instances of objects that use them. Using a weakref in referencing those objects will let them be garbage collected.

@dhomeier dhomeier added the bug label Jan 29, 2025
@dhomeier dhomeier closed this Jan 29, 2025
@dhomeier dhomeier reopened this Jan 29, 2025
@codecov
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.78%. Comparing base (8f8eb9a) to head (f2cadaf).
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #43   +/-   ##
=======================================
  Coverage   96.77%   96.78%           
=======================================
  Files          18       18           
  Lines        2356     2361    +5     
=======================================
+ Hits         2280     2285    +5     
  Misses         76       76           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dhomeier
Copy link
Contributor

Thanks, implementation looks good, though I am not deeply familiar with the use case.

  1. Please rebase this as well to get the linux-ci tests.
  2. Could a test be added to test_containers.py to check the type the callback uses, or maybe even that garbage collection works?

@astrofrog astrofrog force-pushed the fix-weakref-instance-wrappers branch from 864d9d8 to f2cadaf Compare June 11, 2025 22:27
@astrofrog
Copy link
Member

I've rebased, and CI is passing. Adding tests for this kind of issue is not trivial, we try and do this in glue by counting objects with objgraph, but it's a bit messy and sometimes flaky as it depends on the garbage collection. Since all the current tests pass, I think we should merge this, and consider potentially adding tests for the weakref stuff here and elsewhere in echo in future if we encounter bugs related to this.

@astrofrog astrofrog merged commit 0c3ee84 into glue-viz:main Jun 11, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants