Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ref count error issue #906 #907

Merged
merged 11 commits into from
Feb 28, 2020

Conversation

midhun-pm
Copy link
Contributor

PR fixes issue #906

@codecov-io
Copy link

codecov-io commented Feb 20, 2020

Codecov Report

Merging #907 into master will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #907      +/-   ##
==========================================
+ Coverage   72.59%   72.89%   +0.29%     
==========================================
  Files          51       51              
  Lines        6474     6474              
  Branches     1302     1302              
==========================================
+ Hits         4700     4719      +19     
+ Misses       1378     1355      -23     
- Partials      396      400       +4
Impacted Files Coverage Δ
traits/editor_factories.py 81.25% <0%> (-3.13%) ⬇️
traits/trait_types.py 71.07% <0%> (-0.16%) ⬇️
traits/has_traits.py 70.72% <0%> (+0.33%) ⬆️
traits/traits.py 76.92% <0%> (+4.04%) ⬆️
traits/etsconfig/etsconfig.py 63.8% <0%> (+6.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a8a9b1...f705c78. Read the comment docs.

@mdickinson mdickinson self-assigned this Feb 25, 2020
@mdickinson
Copy link
Member

Changes look good. I'll see if I can put together a regression test (but it might take me a few days to get around to it).

@mdickinson
Copy link
Member

I merged the branch for #910 (which contains a regression test) into this one.

@mdickinson
Copy link
Member

Looking at the code, there are some other issues here: for example the PyTuple_GET_SIZE call in the complex trait check should take type_info as an argument, the tests don't cover the various branches for the compound case, and the super call in OldCallable refers to the wrong class. I'll fix all those and push the fixes.

@mdickinson
Copy link
Member

Have expanded the tests somewhat and streamlined the code a bit to test the quicker cases first.

@@ -2921,7 +2921,7 @@ trait_new(PyTypeObject *trait_type, PyObject *args, PyObject *kw)
}

PyErr_Format(
TraitError,
TraitError,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change: VS Code automatically trims trailing whitespace.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sneakily approving my own changes

@mdickinson
Copy link
Member

@midhun-pm I've made a bunch of changes on this PR; please could you review the current state after the changes?

@mdickinson mdickinson assigned midhun-pm and unassigned mdickinson Feb 27, 2020
string_value = "some string"
callable_value = my_function

for _ in range(10):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it 10 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. No really good reason. Let me take out the loop and see if a single iteration is sufficient to cause the segfault.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a single iteration is not enough: on my own machine, I need at least 3. I've added a comment about the number of iterations.

My (unverified) conjecture is that what's happening is that on each iteration through the loop, my_function loses one of its references; when its reference count gets to zero, it gets garbage collected. Everything's fine unless we try to access my_function after it's been garbage collected. The second iteration is probably reducing the reference count to zero, and causing my_function to be garbage collected. The third iteration then accesses my_function and causes the segfault.

@midhun-pm
Copy link
Contributor Author

@midhun-pm I've made a bunch of changes on this PR; please could you review the current state after the changes?

These changes look good. It's clean. Thanks!

@mdickinson mdickinson merged commit d632c04 into master Feb 28, 2020
@mdickinson mdickinson deleted the bugfix/reference_count_callable_allow_none branch February 28, 2020 09:02
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.

3 participants