BugFix: Don't INCREF a Py section just allocated

[ferdonline]

Context

Following #3023, we noticed that there might be extraneous call to INCREF, given it's done on an object returned by tp_alloc, which should return a new reference. New references have their ref count to 1 and therefore it should not require any additional reference increment.

Bug Reproducer

~$ nrniv -python

from neuron import h
h("create soma")
h("objref nc, nil")
h('soma nc = new NetCon(&v(0.5), nil)')
seg = h.nc.preseg()

Exit the interpreter and observe

Fatal Python error: bool_dealloc: deallocating True or False: bug likely caused by a refcount error in a C extension

Scope

• Replace the code by the single function newpysechelp(sec) which seems to do just the right thing.
• Added unit test

Testing

Two python unit tests added:

• test_seg_from_sec_x_ref_python
• test_seg_from_sec_x_ref_hocpy

For debug-ability it might be worth to add the following snippet to seg_from_sec_x

auto* psec = sec->prop->dparam[PROP_PY_INDEX].get<void*>();
fprintf(stderr, "In seg_from_sec_x. Pysec=%p\n", psec);

We will see along the lines of

In seg_from_sec_x. Pysec=0x7fa081736150
In seg_from_sec_x. Pysec=0x0

ensuring the we are exercising both cases.

After dropping the original section, the pure-python code end up with the same number of references as the hoc-python code.

[azure-pipelines]

✔️ 4d66983 -> Azure artifacts URL

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.39%. Comparing base (4d749f1) to head (7842703).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3039   +/-   ##
=======================================
  Coverage   67.38%   67.39%           
=======================================
  Files         573      573           
  Lines      104936   104922   -14     
=======================================
  Hits        70711    70711           
+ Misses      34225    34211   -14     

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

[1uc]

I see the logic behind the change and agree that the INCREF looks very suspicious.

IIUC a section is removed from a model, by letting the python object go out-of-scope. Once the object is GCed it's removed from the model.

We have some tests that assert this. I mostly see RXD tests fail because of this. Also for this PR, but it's failing in exactly the way RXD is known to fail from time to time: #2939.

This leak seems like something that changes the behaviour for users, i.e. it's a bug (because some section would refuse to be deleted from the model).

Do you see a way we could increase our test-coverage to catch the bug?

[ferdonline]

There is something else very suspicious. Notice that we let the created pyseg object get garbage collected (there was a DECREF before). I don't understand the reason to use a Python object basically as a factory for a hoc object... nrnpy_pyobject_in_obj seems to do the right thing though by incrementing the ref count.
Rerunning the failing job indeed made it pass... that's the observed condition then.

It might be worth more people to have a look. In the meantime I'll create a test

[azure-pipelines]

✔️ 79891ce -> Azure artifacts URL

[azure-pipelines]

✔️ 49a70be -> Azure artifacts URL

[azure-pipelines]

✔️ 47b940f -> Azure artifacts URL

[azure-pipelines]

✔️ 7caca61 -> Azure artifacts URL

[ferdonline]

Pls let me know if you consider the test sufficient

[ferdonline]

@nrnhines We need your input in wrt the right behavior of seg_from_sec_x when there is not a corresponding Py Section. Do we want:

• Py ref_count to be 2 and hoc ref_count not changed (current behavior)
• Py ref_count be 1 and hoc ref_count not changed (patch 1)
• Py ref_count be 1 and hoc ref_count incremented (last patch, behavior of newpysechelp)
• ?

[azure-pipelines]

✔️ 04637ce -> Azure artifacts URL

[azure-pipelines]

✔️ 3f50c67 -> Azure artifacts URL

[bbpbuildbot]

Logfiles from GitLab pipeline #229049 (:no_entry:) have been uploaded here!

Status and direct links:

• ✅ mac_m1_cmake_build: [cmake, ON, OFF, OFF, address]
• ✅ spack_setup
• ✅ build:nmodl
• ✅ build:neuron:nmodl:intel:legacy
• ✅ build:neuron:nmodl:intel:shared
• ⛔ build:neuron:nmodl:nvhpc:acc:legacy
• ⛔ build:neuron:nmodl:nvhpc:acc:shared
• ⛔ build:neuron:nmodl:nvhpc:omp:legacy
• ⛔ build:neuron:nmodl:nvhpc:omp
• ✅ test:neuron:nmodl:intel:legacy
• ✅ test:neuron:nmodl:intel:shared
• ⏩ test:neuron:nmodl:nvhpc:acc:legacy
• ⏩ test:neuron:nmodl:nvhpc:acc:shared
• ⏩ test:neuron:nmodl:nvhpc:omp:legacy
• ⏩ test:neuron:nmodl:nvhpc:omp

[azure-pipelines]

✔️ fff7439 -> Azure artifacts URL

[azure-pipelines]

✔️ 349d8a9 -> Azure artifacts URL

[azure-pipelines]

✔️ e64040b -> Azure artifacts URL

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[azure-pipelines]

✔️ 7842703 -> Azure artifacts URL

[1uc]

Very nicely done!