qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Conway <acon...@redhat.com>
Subject Re: qpid-proton git commit: NO-JIRA: Fix core dumps and memory management errors in python binding, engine.py.
Date Fri, 12 Dec 2014 16:45:35 GMT
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, <aconway@apache.org> 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 <aconway@redhat.com>
>         Authored: Thu Dec 11 12:48:42 2014 -0500
>         Committer: Alan Conway <aconway@redhat.com>
>         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: commits-unsubscribe@qpid.apache.org
>         For additional commands, e-mail: commits-help@qpid.apache.org
>         



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


Mime
View raw message