On Fri, 2014-12-12 at 09:50 -0500, Rafael Schloming wrote: > Hi Alan, > > > I'm seeing a whole bunch of errors like this which I believe are a > result of this commit: > > > Exception AttributeError: "'Selectable' object has no attribute > '_impl'" in <bound method Selectable.__del__ of <proton.Selectable > object at 0x122d810>> ignored
Why are those errors ignored? I did fix a bunch of such errors from ctest but obviously missed a few. If the tests had failed instead of ignoring the error I probably wouldn't have missed those. > As an aside, I've just pushed a change that radically simplifies the > memory management strategy from python. It's no longer necessary for > the binding to track all the pointers between parent and child objects > and effectively duplicate the memory management that is already > happening in C. It's possible you ran into those valgrind issues due > to some preliminary work I pushed which might not have been as > thorough as it should have been. As of now the tests should be > valgrind clean modulo a small number of known issues. > > > > I think your strategy of removing the _impl attribute needs to be > adjusted/completed a bit. In the case of Selectable, the attribute is > deleted from an explicit free() method (as opposed to just the __del__ > method) and in this case the code explicitly checks that the attribute > is not None before using it. That check is now failing because the > attribute no longer exists. I think you either need to go back to > assigning it to None or alternatively make sure in all cases where you > delete the attribute that anything that might access it later uses > hasattr rather than just checking for None. I'll back out my change entirely if there are no valgrind errors without them. > > > --Rafael > > > On Thu, Dec 11, 2014 at 1:00 PM, <[email protected]> wrote: > Repository: qpid-proton > Updated Branches: > refs/heads/master d8e99db54 -> e769ad5c8 > > > NO-JIRA: Fix core dumps and memory management errors in > python binding, engine.py. > > Tests were dumping core, valgrind showed pointers being used > after deleted. > > Fix strategy in engine.py: > > Always delete a pointer attribute after freeing it. Any > attempt to use a freed > pointer is an error, by deleting the attribute we detect the > error faster and > with a python trace rather than later on as a core dump in C > code. > > Found and fixed bug in Endpoint.release, was using deleted > pointer. > > > Project: > http://git-wip-us.apache.org/repos/asf/qpid-proton/repo > Commit: > http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/e769ad5c > Tree: > http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/e769ad5c > Diff: > http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/e769ad5c > > Branch: refs/heads/master > Commit: e769ad5c8881feb0ff1e15fc32e5c32aa0006d85 > Parents: d8e99db > Author: Alan Conway <[email protected]> > Authored: Thu Dec 11 12:48:42 2014 -0500 > Committer: Alan Conway <[email protected]> > Committed: Thu Dec 11 12:48:42 2014 -0500 > > ---------------------------------------------------------------------- > proton-c/bindings/python/proton/__init__.py | 31 > +++++++++--------------- > 1 file changed, 12 insertions(+), 19 deletions(-) > ---------------------------------------------------------------------- > > > > http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/e769ad5c/proton-c/bindings/python/proton/__init__.py > ---------------------------------------------------------------------- > diff --git a/proton-c/bindings/python/proton/__init__.py > b/proton-c/bindings/python/proton/__init__.py > index fce3255..356ac4a 100644 > --- a/proton-c/bindings/python/proton/__init__.py > +++ b/proton-c/bindings/python/proton/__init__.py > @@ -1216,7 +1216,7 @@ indicate whether the fd has been > registered or not. > if self._impl: > del self.messenger._selectables[self.fileno()] > pn_selectable_free(self._impl) > - self._impl = None > + del self._impl > > def __del__(self): > self.free() > @@ -2187,10 +2187,11 @@ class Endpoint(object): > def _release(self): > """Release the underlying C Engine resource.""" > if not self._release_invoked: > + conn = self.connection # Releasing the children may > break self.connection. > for c in self._children: > c._release() > self._free_resource() > - self.connection._releasing(self) > + conn._releasing(self) > self._release_invoked = True > > def _update_cond(self): > @@ -2305,17 +2306,13 @@ class Connection(Endpoint): > > def _free_resource(self): > pn_connection_free(self._conn) > - > - def _released(self): > - self._conn = None > + del self._conn > > def _releasing(self, child): > coll = getattr(self, "_collector", None) > if coll: coll = coll() > if coll: > coll._contexts.add(child) > - else: > - child._released() > > def _check(self, err): > if err < 0: > @@ -2433,9 +2430,7 @@ class Session(Endpoint): > > def _free_resource(self): > pn_session_free(self._ssn) > - > - def _released(self): > - self._ssn = None > + del self._ssn > > def free(self): > """Release the Session, freeing its resources. > @@ -2532,10 +2527,8 @@ class Link(Endpoint): > return self._deliveries > > def _free_resource(self): > - pn_link_free(self._link) > - > - def _released(self): > - self._link = None > + if self._link: pn_link_free(self._link) > + del self._link > > def free(self): > """Release the Link, freeing its resources""" > @@ -3013,6 +3006,7 @@ class Transport(object): > if hasattr(self, "_trans"): > if not hasattr(self, "_shared_trans"): > pn_transport_free(self._trans) > + del self._trans > if hasattr(self, "_sasl") and self._sasl: > # pn_transport_free deallocs the C sasl > associated with the > # transport, so erase the reference if a SASL > object was used. > @@ -3022,7 +3016,6 @@ class Transport(object): > # ditto the owned c SSL object > self._ssl._ssl = None > self._ssl = None > - del self._trans > > def _check(self, err): > if err < 0: > @@ -3401,6 +3394,7 @@ class Collector: > > def __del__(self): > pn_collector_free(self._impl) > + del self._impl > > class EventType: > > @@ -3467,7 +3461,6 @@ class Event: > if self.type in (Event.LINK_FINAL, Event.SESSION_FINAL, > Event.CONNECTION_FINAL): > collector._contexts.remove(self.context) > - self.context._released() > > def dispatch(self, handler): > return dispatch(handler, self.type.method, self) > @@ -3573,7 +3566,7 @@ class Connector(object): > if self._cxtr: > pn_connector_set_context(self._cxtr, pn_py2void(None)) > pn_connector_free(self._cxtr) > - self._cxtr = None > + del self._cxtr > > def free(self): > """Release the Connector, freeing its resources. > @@ -3657,7 +3650,7 @@ class Listener(object): > if self._lsnr: > pn_listener_set_context(self._lsnr, pn_py2void(None)); > pn_listener_free(self._lsnr) > - self._lsnr = None > + del self._lsnr > > def free(self): > """Release the Listener, freeing its resources""" > @@ -3821,7 +3814,7 @@ class Url(object): > > def __del__(self): > pn_url_free(self._url); > - self._url = None > + del self._url > > def defaults(self): > """ > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
