You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ac...@apache.org on 2014/12/11 19:00:36 UTC
qpid-proton git commit: NO-JIRA: Fix core dumps and memory management
errors in python binding, engine.py.
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 <ac...@redhat.com>
Authored: Thu Dec 11 12:48:42 2014 -0500
Committer: Alan Conway <ac...@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
Re: qpid-proton git commit: NO-JIRA: Fix core dumps and memory
management errors in python binding, engine.py.
Posted by Alan Conway <ac...@redhat.com>.
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, <ac...@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 <ac...@redhat.com>
> Authored: Thu Dec 11 12:48:42 2014 -0500
> Committer: Alan Conway <ac...@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
Re: qpid-proton git commit: NO-JIRA: Fix core dumps and memory
management errors in python binding, engine.py.
Posted by Rafael Schloming <rh...@alum.mit.edu>.
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, <ac...@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 <ac...@redhat.com>
> Authored: Thu Dec 11 12:48:42 2014 -0500
> Committer: Alan Conway <ac...@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
>
>