Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

## Unreleased
### Added
- Added `getMemUsed()`, `getMemTotal()`, and `getMemExternEstim()` methods
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably that we do not have to worry about memory anymore because we know how to measure it.

### Fixed
- Removed incorrect `Py_INCREF`/`Py_DECREF` on `Model` in `catchEvent`/`dropEvent` that caused reference count imbalance
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Removed incorrect `Py_INCREF`/`Py_DECREF` on `Model` in `catchEvent`/`dropEvent` that caused reference count imbalance
- Removed `Py_INCREF`/`Py_DECREF` on `Model` in `catchEvent`/`dropEvent` that caused memory leak for imbalanced usage

### Changed
### Removed

Expand Down
2 changes: 0 additions & 2 deletions src/pyscipopt/scip.pxi
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@
if rc == SCIP_OKAY:
pass
elif rc == SCIP_ERROR:
raise Exception('SCIP: unspecified error!')

Check failure on line 319 in src/pyscipopt/scip.pxi

View workflow job for this annotation

GitHub Actions / test-coverage (3.11)

SCIP: unspecified error!
elif rc == SCIP_NOMEMORY:
raise MemoryError('SCIP: insufficient memory error!')
elif rc == SCIP_READERROR:
Expand All @@ -335,7 +335,7 @@
raise Exception('SCIP: method cannot be called at this time'
+ ' in solution process!')
elif rc == SCIP_INVALIDDATA:
raise Exception('SCIP: error in input data!')

Check failure on line 338 in src/pyscipopt/scip.pxi

View workflow job for this annotation

GitHub Actions / test-coverage (3.11)

SCIP: error in input data!
elif rc == SCIP_INVALIDRESULT:
raise Exception('SCIP: method returned an invalid result code!')
elif rc == SCIP_PLUGINNOTFOUND:
Expand Down Expand Up @@ -11268,7 +11268,6 @@
else:
raise Warning("event handler not found")

Py_INCREF(self)
PY_SCIP_CALL(SCIPcatchEvent(self._scip, eventtype, _eventhdlr, NULL, NULL))

def dropEvent(self, eventtype, Eventhdlr eventhdlr):
Expand All @@ -11288,7 +11287,6 @@
else:
raise Warning("event handler not found")

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

def catchVarEvent(self, Variable var, eventtype, Eventhdlr eventhdlr):
Expand Down
40 changes: 38 additions & 2 deletions tests/test_event.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import pytest, random
import pytest, weakref, gc, random

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

Expand Down Expand Up @@ -189,4 +189,40 @@ def eventexec(self, event):
m.includeEventhdlr(ev, "var_event", "event handler for var events")

with pytest.raises(Exception):
m.optimize()
m.optimize()

def test_catchEvent_does_not_leak_model():
"""catchEvent should not artificially increment the Model's reference count.

Previously, catchEvent called Py_INCREF(self) on the Model, and dropEvent
called Py_DECREF(self). Since many event handlers skip dropEvent (e.g. when
self.model is already dead), this caused the Model to leak — its refcount
never returned to zero, preventing garbage collection.
Comment on lines +199 to +200
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence feels quite incomplete, but well.

"""

class SimpleEvent(Eventhdlr):
def eventinit(self):
self.model.catchEvent(SCIP_EVENTTYPE.NODEFOCUSED, self)

def eventexit(self):
pass # intentionally no dropEvent, which is bad practice

def eventexec(self, event):
pass

m = Model()
m.hideOutput()
ev = SimpleEvent()
m.includeEventhdlr(ev, "simple", "test event handler")
m.addVar("x", obj=1, vtype="I")
m.optimize()

ref = weakref.ref(m)

del ev
gc.collect()
assert ref() is not None, "Model was garbage collected — event handler absorbed a reference"

del m
gc.collect()
assert ref() is None, "Model was not garbage collected — catchEvent likely leaked a reference"