#12313: Fix yet another memory leak caused by caching of coercion data
--------------------------------------------------+-------------------------
Reporter: SimonKing | Owner:
Type: defect | Status:
needs_review
Priority: major | Milestone: sage-5.3
Component: memleak | Resolution:
Keywords: coercion weak dictionary | Work issues:
Report Upstream: N/A | Reviewers: Simon King,
Jean-Pierre Flori, John Perry
Authors: Simon King, Jean-Pierre Flori | Merged in:
Dependencies: #11521, #11599, #12969, #12215 | Stopgaps:
--------------------------------------------------+-------------------------
Comment (by nbruin):
Replying to [comment:238 jpflori]:
> I'm not yet convinced the explanation is as what I posted above, but
here is a patch which seems logical to me, and prevents the polybori
segfaults.
I agree with your assessment as well. As far as I can see:
`GroebnerStrategy.__init__`:
- Creates a new `ReductionStrategy` wrapper and stores it in
`self.reduction_strategy`
- Asks polybori to delete the `self.reduction_strategy._strat` attribute
on it
- Then fills that slot with `&self._strat.generators`
So `GroebnerStrategy.reduction_strategy._strat` is now definitely a
borrowed reference from `ReductionStrategy`. However, `GroebnerStrategy`
has a reference to the latter, so it's borrowing from something it keeps
alive.
`GroebnerStrategy.__dealloc__`:
- zeros out `self.reduction_strategy._strat = NULL`
- calls `del self._strat`, which I assume amounts to asking polybori to
delete it as well.
This strategy works if `GroebnerStrategy.__dealloc__` would get called
before `ReductionStrategy.__dealloc`, but as we know python attributes
might already no longer exist at `__dealloc__` time, so we're not
guaranteed a call order.
So I think the code here should be
{{{
if self.reduction_strategy != NULL and self.reduction_strategy._strat
!= NULL:
self.reduction_strategy._strat = NULL
if self._count.released():
del self._strat
}}}
but this is assuming that attributes get zeroed out on deallocation (Can
we even assume that `self.reductio_strategy` is still alive at this
point?)
Then you could do in `ReductionStrategy.__dealloc__`:
{{{
if self._strat:
PBRedStrategy_delete(self._strat)
self._strat = NULL
}}}
However, that's assuming that we can establish whether
`GroebnerStrategy.reduction_strategy` is still valid upon deallocation.
Since `GroebnerStrategy` has borrowed the reference without
`ReductionStrategy` really knowing, I think `GroebnerStrategy` should just
not delete `_strat`. In due time, `ReductionStrategy.__dealloc__` gets
called and will clean up the ref. It's hers!
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/12313#comment:241>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
