memory leak fix; Py_XDCREF amp_func set to NULL

[hammy4815]

closes #2379
closes #2381

As discussed, adding NULL assignments in meep.i after the calls to Py_XDECREF fixes the issue.

[smartalecH]

Nice! Can we add a quick test?

[smartalecH]

(In your description, rather than saying "Ref issue" say "closes" so that a GitHub hook will close that issue after this is merged. You can close multiple issues/PRs that way so maybe include #2381)

[smartalecH]

(cc @Arjun-Khurana)

[hammy4815]

Nice! Can we add a quick test?

Should the test simply create a Source with an amp_func and quick run, then change sources to an EigenModeSource with a quick run, then change back and ensure a quick run does not break?

[oskooi]

Should the test simply create a Source with an amp_func and quick run, then change sources to an EigenModeSource with a quick run, then change back and ensure a quick run does not break?

That seems reasonable. But perhaps @Arjun-Khurana can first confirm this patch fixes the segmentation fault that he reported in #2379?

[stevengj]

First you need something that reliably reproduces the probelm on master … then strip it down to as quick a simulation as possible (e.g. set the resolution, cell size, and runtime, to be tiny).

[stevengj]

Maybe add something to https://github.com/NanoComp/meep/blob/master/python/tests/test_source.py

… that is, before one of the existing tests, add a some source, then change the source to the correct source for the test. This will verify not only that it doesn't crash, but also that changing the source actually results in the source being changed as requested.

[Arjun-Khurana]

I wrote a unit test for this on my fork.

All it does is spawn a child process, run the MWE from #2379 and assert that the exit code is 0, which is false if the segfault occurs.

#2381 also has these changes reflected and includes that unit test, but since it is poorly named relative to the actual implementation of the fix (and thus closed), you can feel free to include/modify that test in this PR @hammy4815 .

[hammy4815]

Thanks @Arjun-Khurana ,

I'll take a look at your unit test and push a test here. I think we're planning to integrate this test into an existing in test_source.py to avoid making a new test file for this case. Once I'm done we can merge and close.

[hammy4815]

The bug is fixed as we established, however, I believe something is missing in our understanding. Using the optimization object, we can recreate the issue with ease. To our understanding, this issue should still exist even without the use of an optimization. The following test should fail, yet does not.

 def test_amp_func_change_sources(self):
        src = ContinuousSource(5.0)
        center = Vector3()
        size = Vector3(0, 1, 0)

        ampfunc = lambda X: 1.0
        amp_source = [Source(src, component=mp.Ez, center=center, size=size, amp_func=ampfunc)]
        sim = mp.Simulation(cell_size=Vector3(1, 1, 0), resolution=5, sources=amp_source)
        sim.run(until=1)

        sim.restart_fields()
        sim.clear_dft_monitors()
        default_lattice = [EigenModeSource(src, size=size, center=center)]
        sim.change_sources(default_lattice)
        sim.run(until=1)

        sim.reset_meep()
        sim.change_sources(amp_source)
        sim.run(until=1)

        self.assertTrue((sim.sources[0].amp_func is ampfunc))
        self.assertTrue((sim.sources[0].amp_func is not None))

Here, when I call sim.change_sources(default_lattice), the nested calls down to

meep/python/source.py

Lines 671 to 674 in 395d821

	if isinstance(self.eig_band, mp.DiffractedPlanewave):
	add_eig_src(self.amp_func, diffractedplanewave)
	else:
	add_eig_src(self.amp_func)

gets called and add_eig_src(self.amp_func) does not delete the other amp_func like it does when it gets called from the optimization interface.

From here we can either 1) try to find out what is happening in optimization to allow the trigger of the memory leak down the line, or 2) accept that the leak is happening, but we fixed it anyways with the Null assignments so make the unit test with an optimization object like what's done in Arjun's test.

[hammy4815]

For whatever reason, the issue only occurs when the list of sources entering change_sources contains at least two EigenModeSources:

        default_lattice = [EigenModeSource(src, size=size, center=center),
                           EigenModeSource(src, size=size, center=center)]
        sim.change_sources(default_lattice)

I will include this in the test, and then push to call this final, unless we want to explore this.

[codecov-commenter]

Codecov Report

Merging #2394 (22dc9ca) into master (b86b476) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #2394   +/-   ##
=======================================
  Coverage   72.57%   72.57%           
=======================================
  Files          17       17           
  Lines        5166     5166           
=======================================
  Hits         3749     3749           
  Misses       1417     1417           

Impacted Files	Coverage Δ
python/adjoint/filters.py	60.58% <0.00%> (-0.29%)	⬇️
python/geom.py	94.15% <0.00%> (+0.01%)	⬆️