Skip to content

Commit b31fc2c

Browse files
Remove incorrect Py_INCREF/DECREF on Model in catchEvent/dropEvent (#1190)
* Remove spurious Py_INCREF/DECREF on Model in catchEvent/dropEvent * Apply suggestions from code review Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> * Update tests/test_event.py Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> * Update tests/test_event.py Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> * Add changelog entries for unreleased changes * Fix incomplete docstring in test_catchEvent_does_not_leak_model * Update CHANGELOG.md Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> --------- Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com>
1 parent 3b49268 commit b31fc2c

File tree

3 files changed

+42
-4
lines changed

3 files changed

+42
-4
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22

33
## Unreleased
44
### Added
5+
- Added `getMemUsed()`, `getMemTotal()`, and `getMemExternEstim()` methods
56
### Fixed
7+
- Removed `Py_INCREF`/`Py_DECREF` on `Model` in `catchEvent`/`dropEvent` that caused memory leak for imbalanced usage
68
### Changed
79
### Removed
810

src/pyscipopt/scip.pxi

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11268,7 +11268,6 @@ cdef class Model:
1126811268
else:
1126911269
raise Warning("event handler not found")
1127011270

11271-
Py_INCREF(self)
1127211271
PY_SCIP_CALL(SCIPcatchEvent(self._scip, eventtype, _eventhdlr, NULL, NULL))
1127311272

1127411273
def dropEvent(self, eventtype, Eventhdlr eventhdlr):
@@ -11288,7 +11287,6 @@ cdef class Model:
1128811287
else:
1128911288
raise Warning("event handler not found")
1129011289

11291-
Py_DECREF(self)
1129211290
PY_SCIP_CALL(SCIPdropEvent(self._scip, eventtype, _eventhdlr, NULL, -1))
1129311291

1129411292
def catchVarEvent(self, Variable var, eventtype, Eventhdlr eventhdlr):

tests/test_event.py

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import pytest, random
1+
import pytest, weakref, gc, random
22

33
from pyscipopt import Model, Eventhdlr, SCIP_RESULT, SCIP_EVENTTYPE, SCIP_PARAMSETTING, quicksum
44

@@ -189,4 +189,42 @@ def eventexec(self, event):
189189
m.includeEventhdlr(ev, "var_event", "event handler for var events")
190190

191191
with pytest.raises(Exception):
192-
m.optimize()
192+
m.optimize()
193+
194+
def test_catchEvent_does_not_leak_model():
195+
"""catchEvent should not artificially increment the Model's reference count.
196+
197+
Previously, catchEvent called Py_INCREF(self) on the Model, and dropEvent
198+
called Py_DECREF(self). In practice these calls are often unbalanced — event
199+
handlers commonly catch events without a matching drop (e.g. calling
200+
catchEvent in eventinit but omitting dropEvent in eventexit). Each unmatched
201+
catchEvent permanently inflated the Model's refcount, preventing garbage
202+
collection.
203+
"""
204+
205+
class SimpleEvent(Eventhdlr):
206+
def eventinit(self):
207+
self.model.catchEvent(SCIP_EVENTTYPE.NODEFOCUSED, self)
208+
209+
def eventexit(self):
210+
pass # intentionally no dropEvent, which is bad practice
211+
212+
def eventexec(self, event):
213+
pass
214+
215+
m = Model()
216+
m.hideOutput()
217+
ev = SimpleEvent()
218+
m.includeEventhdlr(ev, "simple", "test event handler")
219+
m.addVar("x", obj=1, vtype="I")
220+
m.optimize()
221+
222+
ref = weakref.ref(m)
223+
224+
del ev
225+
gc.collect()
226+
assert ref() is not None, "Model was garbage collected — event handler absorbed a reference"
227+
228+
del m
229+
gc.collect()
230+
assert ref() is None, "Model was not garbage collected — catchEvent likely leaked a reference"

0 commit comments

Comments
 (0)