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 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. --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] > >
