Replace weakref.proxy with strong references for plugin self.model (#1193)

Author: Joao-Dionisio

CHANGELOG.md

@@ -5,6 +5,7 @@
 - Added `getMemUsed()`, `getMemTotal()`, and `getMemExternEstim()` methods
 ### Fixed
 - Removed `Py_INCREF`/`Py_DECREF` on `Model` in `catchEvent`/`dropEvent` that caused memory leak for imbalanced usage
+- Replaced `weakref.proxy` with strong references for plugin `self.model`, fixing `ReferenceError` during cleanup callbacks (#1193)
 ### Changed
 ### Removed
 

src/pyscipopt/scip.pxd

@@ -2249,11 +2249,14 @@ cdef class Model:
     cdef int _generated_event_handlers_count
     # store references to Benders subproblem Models for proper cleanup
     cdef _benders_subproblems
+    # store references to plugins to break circular references in __dealloc__
+    cdef _plugins
     # store iis, if found
     cdef SCIP_IIS* _iis
     # helper methods for later var and cons cleanup
     cdef _getOrCreateCons(self, SCIP_CONS* scip_cons)
     cdef _getOrCreateVar(self, SCIP_VAR* scip_var)
+    cdef _free_scip_instance(self)
 
     @staticmethod
     cdef create(SCIP* scip)

src/pyscipopt/scip.pxi

@@ -1,6 +1,5 @@
 ##@file scip.pxi
 #@brief holding functions in python that reference the SCIP public functions included in scip.pxd
-import weakref
 from os.path import abspath
 from os.path import splitext
 import os
@@ -2821,6 +2820,7 @@ cdef class Model:
         self._modelconss = {}
         self._generated_event_handlers_count = 0
         self._benders_subproblems = []  # Keep references to Benders subproblem Models
+        self._plugins = []  # Keep references to plugins to break cycles in __dealloc__
         self._iis = NULL
 
         if not createscip:
@@ -2892,15 +2892,30 @@ cdef class Model:
 
         self.includeEventhdlr(event_handler, name, description)
 
+    def __del__(self):
+        """Free SCIP before the cyclic GC clears Python references.
+
+        __del__ (tp_finalize) runs before tp_clear, so _plugins and other
+        Python attributes are still intact. This lets SCIPfree call plugin
+        callbacks (consfree, eventexit, etc.) safely. The GC handles breaking
+        the circular references afterwards via tp_clear.
+        """
+        self._free_scip_instance()
+
     def __dealloc__(self):
-        # Declare all C variables at the beginning for Cython compatibility
+        # Safety net: if __del__ didn't run (e.g. interpreter shutdown),
+        # free SCIP here. Plugin callbacks may not work at this point since
+        # tp_clear may have already cleared _plugins.
+        if self._scip is not NULL and self._freescip and PY_SCIP_CALL:
+            PY_SCIP_CALL( SCIPfree(&self._scip) )
+
+    cdef _free_scip_instance(self):
+        """Free the SCIP instance. Does not touch Python object references."""
         cdef SCIP_BENDERS** benders
         cdef int nbenders
         cdef int nsubproblems
         cdef int i, j
 
-        # call C function directly, because we can no longer call this object's methods, according to
-        # http://docs.cython.org/src/reference/extension_types.html#finalization-dealloc
         if self._scip is not NULL and self._freescip and PY_SCIP_CALL:
             # Free Benders subproblems before freeing the main SCIP instance
             if self._benders_subproblems:
@@ -2912,20 +2927,17 @@ cdef class Model:
                     for j in range(nsubproblems):
                         PY_SCIP_CALL(SCIPfreeBendersSubproblem(self._scip, benders[i], j))
 
-                # Clear the references to allow Python GC to clean up the Model objects
-                self._benders_subproblems = []
+            SCIPfree(&self._scip)
+            self._scip = NULL
+            self._freescip = False
 
-            # Invalidate all variable and constraint pointers before freeing SCIP. See issue #604.
+            # Invalidate all variable and constraint pointers after freeing SCIP. See issue #604.
             if self._modelvars:
                 for var in self._modelvars.values():
                     (<Variable>var).scip_var = NULL
-                self._modelvars = {}
             if self._modelconss:
                 for cons in self._modelconss.values():
                     (<Constraint>cons).scip_cons = NULL
-                self._modelconss = {}
-
-            PY_SCIP_CALL( SCIPfree(&self._scip) )
 
     def __hash__(self):
         return hash(<size_t>self._scip)
@@ -3070,6 +3082,21 @@ cdef class Model:
         """Frees problem and solution process data."""
         PY_SCIP_CALL(SCIPfreeProb(self._scip))
 
+    def free(self):
+        """Explicitly free SCIP instance and break circular references with plugins.
+
+        This method should be called when you want to ensure deterministic cleanup.
+        All SCIP callbacks (consfree, eventexit, etc.) are executed during this call.
+        After calling this method, the Model object should not be used anymore.
+        """
+        self._free_scip_instance()
+
+        # Break circular references with plugins
+        if self._plugins:
+            for plugin in self._plugins:
+                plugin.model = None
+            self._plugins = []
+
     def freeTransform(self):
         """Frees all solution process data including presolving and
         transformed problem, only original problem is kept."""
@@ -9191,9 +9218,9 @@ cdef class Model:
                                           PyEventDelete,
                                           PyEventExec,
                                           <SCIP_EVENTHDLRDATA*>eventhdlr))
-        eventhdlr.model = <Model>weakref.proxy(self)
+        eventhdlr.model = self
         eventhdlr.name = name
-        Py_INCREF(eventhdlr)
+        self._plugins.append(eventhdlr)
 
     def includePricer(self, Pricer pricer, name, desc, priority=1, delay=True):
         """
@@ -9223,8 +9250,8 @@ cdef class Model:
         cdef SCIP_PRICER* scip_pricer
         scip_pricer = SCIPfindPricer(self._scip, n)
         PY_SCIP_CALL(SCIPactivatePricer(self._scip, scip_pricer))
-        pricer.model = <Model>weakref.proxy(self)
-        Py_INCREF(pricer)
+        pricer.model = self
+        self._plugins.append(pricer)
         pricer.scip_pricer = scip_pricer
 
     def includeConshdlr(self, Conshdlr conshdlr, name, desc, sepapriority=0,
@@ -9283,9 +9310,9 @@ cdef class Model:
                                               PyConsActive, PyConsDeactive, PyConsEnable, PyConsDisable, PyConsDelvars, PyConsPrint, PyConsCopy,
                                               PyConsParse, PyConsGetvars, PyConsGetnvars, PyConsGetdivebdchgs, PyConsGetPermSymGraph, PyConsGetSignedPermSymGraph,
                                               <SCIP_CONSHDLRDATA*>conshdlr))
-        conshdlr.model = <Model>weakref.proxy(self)
+        conshdlr.model = self
         conshdlr.name = name
-        Py_INCREF(conshdlr)
+        self._plugins.append(conshdlr)
 
     def deactivatePricer(self, Pricer pricer):
         """
@@ -9450,8 +9477,8 @@ cdef class Model:
         d = str_conversion(desc)
         PY_SCIP_CALL(SCIPincludePresol(self._scip, n, d, priority, maxrounds, timing, PyPresolCopy, PyPresolFree, PyPresolInit,
                                             PyPresolExit, PyPresolInitpre, PyPresolExitpre, PyPresolExec, <SCIP_PRESOLDATA*>presol))
-        presol.model = <Model>weakref.proxy(self)
-        Py_INCREF(presol)
+        presol.model = self
+        self._plugins.append(presol)
 
     def includeSepa(self, Sepa sepa, name, desc, priority=0, freq=10, maxbounddist=1.0, usessubscip=False, delay=False):
         """
@@ -9493,9 +9520,9 @@ cdef class Model:
         d = str_conversion(desc)
         PY_SCIP_CALL(SCIPincludeSepa(self._scip, n, d, priority, freq, maxbounddist, usessubscip, delay, PySepaCopy, PySepaFree,
                                           PySepaInit, PySepaExit, PySepaInitsol, PySepaExitsol, PySepaExeclp, PySepaExecsol, <SCIP_SEPADATA*>sepa))
-        sepa.model = <Model>weakref.proxy(self)
+        sepa.model = self
         sepa.name = name
-        Py_INCREF(sepa)
+        self._plugins.append(sepa)
 
     def includeReader(self, Reader reader, name, desc, ext):
         """
@@ -9518,9 +9545,9 @@ cdef class Model:
         e = str_conversion(ext)
         PY_SCIP_CALL(SCIPincludeReader(self._scip, n, d, e, PyReaderCopy, PyReaderFree,
                                           PyReaderRead, PyReaderWrite, <SCIP_READERDATA*>reader))
-        reader.model = <Model>weakref.proxy(self)
+        reader.model = self
         reader.name = name
-        Py_INCREF(reader)
+        self._plugins.append(reader)
 
     def includeProp(self, Prop prop, name, desc, presolpriority, presolmaxrounds,
                     proptiming, presoltiming=SCIP_PRESOLTIMING_FAST, priority=1, freq=1, delay=True):
@@ -9560,8 +9587,8 @@ cdef class Model:
                                           PyPropInitpre, PyPropExitpre, PyPropInitsol, PyPropExitsol,
                                           PyPropPresol, PyPropExec, PyPropResProp,
                                           <SCIP_PROPDATA*> prop))
-        prop.model = <Model>weakref.proxy(self)
-        Py_INCREF(prop)
+        prop.model = self
+        self._plugins.append(prop)
 
     def includeHeur(self, Heur heur, name, desc, dispchar, priority=10000, freq=1, freqofs=0,
                     maxdepth=-1, timingmask=SCIP_HEURTIMING_BEFORENODE, usessubscip=False):
@@ -9605,9 +9632,9 @@ cdef class Model:
                                           PyHeurCopy, PyHeurFree, PyHeurInit, PyHeurExit,
                                           PyHeurInitsol, PyHeurExitsol, PyHeurExec,
                                           <SCIP_HEURDATA*> heur))
-        heur.model = <Model>weakref.proxy(self)
+        heur.model = self
         heur.name = name
-        Py_INCREF(heur)
+        self._plugins.append(heur)
 
     def includeIISfinder(self, IISfinder iisfinder, name, desc, priority=10000, freq=1):
         """
@@ -9639,7 +9666,7 @@ cdef class Model:
 
         scip_iisfinder = SCIPfindIISfinder(self._scip, nam)
         iisfinder.name = name
-        Py_INCREF(iisfinder)
+        self._plugins.append(iisfinder)
         iisfinder.scip_iisfinder = scip_iisfinder
 
     def generateIIS(self):
@@ -9697,10 +9724,9 @@ cdef class Model:
         des = str_conversion(desc)
         PY_SCIP_CALL(SCIPincludeRelax(self._scip, nam, des, priority, freq, PyRelaxCopy, PyRelaxFree, PyRelaxInit, PyRelaxExit,
                                           PyRelaxInitsol, PyRelaxExitsol, PyRelaxExec, <SCIP_RELAXDATA*> relax))
-        relax.model = <Model>weakref.proxy(self)
+        relax.model = self
         relax.name = name
-
-        Py_INCREF(relax)
+        self._plugins.append(relax)
 
     def includeCutsel(self, Cutsel cutsel, name, desc, priority):
         """
@@ -9725,8 +9751,8 @@ cdef class Model:
                                        priority, PyCutselCopy, PyCutselFree, PyCutselInit, PyCutselExit,
                                        PyCutselInitsol, PyCutselExitsol, PyCutselSelect,
                                        <SCIP_CUTSELDATA*> cutsel))
-        cutsel.model = <Model>weakref.proxy(self)
-        Py_INCREF(cutsel)
+        cutsel.model = self
+        self._plugins.append(cutsel)
 
     def includeBranchrule(self, Branchrule branchrule, name, desc, priority, maxdepth, maxbounddist):
         """
@@ -9757,8 +9783,8 @@ cdef class Model:
                                           PyBranchruleCopy, PyBranchruleFree, PyBranchruleInit, PyBranchruleExit,
                                           PyBranchruleInitsol, PyBranchruleExitsol, PyBranchruleExeclp, PyBranchruleExecext,
                                           PyBranchruleExecps, <SCIP_BRANCHRULEDATA*> branchrule))
-        branchrule.model = <Model>weakref.proxy(self)
-        Py_INCREF(branchrule)
+        branchrule.model = self
+        self._plugins.append(branchrule)
 
     def includeNodesel(self, Nodesel nodesel, name, desc, stdpriority, memsavepriority):
         """
@@ -9785,8 +9811,8 @@ cdef class Model:
                                           PyNodeselCopy, PyNodeselFree, PyNodeselInit, PyNodeselExit,
                                           PyNodeselInitsol, PyNodeselExitsol, PyNodeselSelect, PyNodeselComp,
                                           <SCIP_NODESELDATA*> nodesel))
-        nodesel.model = <Model>weakref.proxy(self)
-        Py_INCREF(nodesel)
+        nodesel.model = self
+        self._plugins.append(nodesel)
 
     def includeBenders(self, Benders benders, name, desc, priority=1, cutlp=True, cutpseudo=True, cutrelax=True,
             shareaux=False):
@@ -9825,10 +9851,10 @@ cdef class Model:
                                             <SCIP_BENDERSDATA*>benders))
         cdef SCIP_BENDERS* scip_benders
         scip_benders = SCIPfindBenders(self._scip, n)
-        benders.model = <Model>weakref.proxy(self)
+        benders.model = self
         benders.name = name
         benders._benders = scip_benders
-        Py_INCREF(benders)
+        self._plugins.append(benders)
 
     def includeBenderscut(self, Benders benders, Benderscut benderscut, name, desc, priority=1, islpcut=True):
         """
@@ -9864,11 +9890,10 @@ cdef class Model:
 
         cdef SCIP_BENDERSCUT* scip_benderscut
         scip_benderscut = SCIPfindBenderscut(_benders, n)
-        benderscut.model = <Model>weakref.proxy(self)
+        benderscut.model = self
         benderscut.benders = benders
         benderscut.name = name
-        # TODO: It might be necessary in increment the reference to benders i.e Py_INCREF(benders)
-        Py_INCREF(benderscut)
+        self._plugins.append(benderscut)
 
     def getLPBranchCands(self):
         """

tests/test_conshdlr.py

@@ -68,11 +68,7 @@ def conslock(self, constraint, locktype, nlockspos, nlocksneg):
         calls.add("conslock")
         assert id(constraint) in ids
 
-        try:
-            var = self.model.data["x"]
-        except ReferenceError:
-            return
-
+        var = self.model.data["x"]
         n_locks_down = var.getNLocksDown()
         n_locks_up = var.getNLocksUp()
         n_locks_down_model = var.getNLocksDownType(SCIP_LOCKTYPE.MODEL)
@@ -249,8 +245,9 @@ def create_model():
     # solve problem
     model.optimize()
 
-    # so that consfree gets called
+    # so that consfree gets called (GC collects the Model ↔ plugin cycle)
     del model
+    import gc; gc.collect()
 
     # check callbacks got called
     assert "consenfolp" in calls