You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2014/01/24 21:34:23 UTC
Review Request 17329: [proton-c] Manage references properly to allow
resource deallocation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17329/
-----------------------------------------------------------
Review request for qpid, Gordon Sim and Rafael Schloming.
Bugs: proton-489
https://issues.apache.org/jira/browse/proton-489
Repository: qpid
Description
-------
Prior to this fix, freeing an endpoint would leave stale pointers in referring objects. This fix makes use of the object reference functionality to ensure an endpoint is only removed when all references to it are dropped.
Diffs
-----
/proton/trunk/proton-c/include/proton/object.h 1554940
/proton/trunk/proton-c/src/engine/engine.c 1554940
/proton/trunk/proton-c/src/messenger/messenger.c 1554940
/proton/trunk/proton-c/src/object/object.c 1554940
/proton/trunk/proton-c/src/transport/transport.h 1554940
/proton/trunk/proton-c/src/transport/transport.c 1554940
Diff: https://reviews.apache.org/r/17329/diff/
Testing
-------
Unit tests, including valgrind tests.
Thanks,
Kenneth Giusti
Re: Review Request 17329: [proton-c] Manage references properly to
allow resource deallocation.
Posted by Rafael Schloming <rh...@apache.org>.
On Jan 27, 2014 12:26 PM, "Kenneth Giusti" <kg...@apache.org> wrote:
>
>
>
> > On Jan. 27, 2014, 3:06 p.m., Rafael Schloming wrote:
> > > /proton/trunk/proton-c/src/engine/engine.c, line 91
> > > <
https://reviews.apache.org/r/17329/diff/2/?file=451420#file451420line91>
> > >
> > > I'm putting this comment here because it kind of relates to
changing the pn_free to a pn_decref, but really it's about the overall
diff. Assuming I'm reading it correctly, I believe you have modified the
ref counting semantics such that holding a pointer to a child object
prevents the parent from being collected. While this makes sense from a
python perspective, this actually makes the C API far more brittle since I
can't simply free the connection and be sure that it and all of its
components go away. I need to ensure that I track and free every single
child object that I've created. This seems kind of annoying since the
connection is tracking all that stuff anyways.
> > >
> > > I thought the idea was that the connection level free blows away
everything, but that the python binding keeps parent pointers to prevent
the connection from being blown away.
> > >
> > > That said, I don't think it's necessarily bad to implement these
kind of semantics in C if it means less work in the bindings, however I
think there should still be a connection level free that blows everything
away if only for situations in C where you want to robustly clean up.
>
> > Assuming I'm reading it correctly, I believe you have modified the ref
counting semantics such that holding a pointer to a child object prevents
the parent from being collected.
>
> While it is true that this patch would cause the parent from being
collected, that's really not the motivation for this patch. My intent is
to use precise reference counting to manage the lifecycle of endpoint
objects. But you're correct - by introducing this pattern we force the
parent to exist as long as the child holds a pointer to it, since each
child currently holds a pointer to the parent.
>
> I was really trying to address a seg-fault that was caused by the
behavior that you want to keep - freeing everything when the connection is
freed. Specifically:
>
> pn_connection_free(conn)
> pn_session_free(ssn) <-- segfault if ssn over conn
>
> So doing this is not really a proton bug - it's a programming error?
Ah, I thought the bug you were fixing was a seg fault due to freeing a
session before it's link or a link before it's delivery.
I would say in general the whole graph of objects contained within a
connection should follow proper ref counting semantics as you describe,
however we need to be clear on whether the connection object itself is a
node in the graph or whether it is a container of all the nodes in the
graph, or in fact if we need both sets of semantics somehow.
>
>
> - Kenneth
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17329/#review32832
> -----------------------------------------------------------
>
>
> On Jan. 27, 2014, 1:33 p.m., Kenneth Giusti wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/17329/
> > -----------------------------------------------------------
> >
> > (Updated Jan. 27, 2014, 1:33 p.m.)
> >
> >
> > Review request for qpid, Gordon Sim and Rafael Schloming.
> >
> >
> > Bugs: proton-489
> > https://issues.apache.org/jira/browse/proton-489
> >
> >
> > Repository: qpid
> >
> >
> > Description
> > -------
> >
> > Prior to this fix, freeing an endpoint would leave stale pointers in
referring objects. This fix makes use of the object reference
functionality to ensure an endpoint is only removed when all references to
it are dropped.
> >
> >
> > Diffs
> > -----
> >
> > /proton/trunk/proton-c/include/proton/object.h 1560765
> > /proton/trunk/proton-c/src/engine/engine.c 1560765
> > /proton/trunk/proton-c/src/messenger/messenger.c 1560765
> > /proton/trunk/proton-c/src/object/object.c 1560765
> > /proton/trunk/proton-c/src/transport/transport.h 1560765
> > /proton/trunk/proton-c/src/transport/transport.c 1560765
> >
> > Diff: https://reviews.apache.org/r/17329/diff/
> >
> >
> > Testing
> > -------
> >
> > Unit tests, including valgrind tests.
> >
> >
> > Thanks,
> >
> > Kenneth Giusti
> >
> >
>
Re: Review Request 17329: [proton-c] Manage references properly to allow
resource deallocation.
Posted by Kenneth Giusti <kg...@apache.org>.
> On Jan. 27, 2014, 3:06 p.m., Rafael Schloming wrote:
> > /proton/trunk/proton-c/src/engine/engine.c, line 91
> > <https://reviews.apache.org/r/17329/diff/2/?file=451420#file451420line91>
> >
> > I'm putting this comment here because it kind of relates to changing the pn_free to a pn_decref, but really it's about the overall diff. Assuming I'm reading it correctly, I believe you have modified the ref counting semantics such that holding a pointer to a child object prevents the parent from being collected. While this makes sense from a python perspective, this actually makes the C API far more brittle since I can't simply free the connection and be sure that it and all of its components go away. I need to ensure that I track and free every single child object that I've created. This seems kind of annoying since the connection is tracking all that stuff anyways.
> >
> > I thought the idea was that the connection level free blows away everything, but that the python binding keeps parent pointers to prevent the connection from being blown away.
> >
> > That said, I don't think it's necessarily bad to implement these kind of semantics in C if it means less work in the bindings, however I think there should still be a connection level free that blows everything away if only for situations in C where you want to robustly clean up.
> Assuming I'm reading it correctly, I believe you have modified the ref counting semantics such that holding a pointer to a child object prevents the parent from being collected.
While it is true that this patch would cause the parent from being collected, that's really not the motivation for this patch. My intent is to use precise reference counting to manage the lifecycle of endpoint objects. But you're correct - by introducing this pattern we force the parent to exist as long as the child holds a pointer to it, since each child currently holds a pointer to the parent.
I was really trying to address a seg-fault that was caused by the behavior that you want to keep - freeing everything when the connection is freed. Specifically:
pn_connection_free(conn)
pn_session_free(ssn) <-- segfault if ssn over conn
So doing this is not really a proton bug - it's a programming error?
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17329/#review32832
-----------------------------------------------------------
On Jan. 27, 2014, 1:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17329/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 1:33 p.m.)
>
>
> Review request for qpid, Gordon Sim and Rafael Schloming.
>
>
> Bugs: proton-489
> https://issues.apache.org/jira/browse/proton-489
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Prior to this fix, freeing an endpoint would leave stale pointers in referring objects. This fix makes use of the object reference functionality to ensure an endpoint is only removed when all references to it are dropped.
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/include/proton/object.h 1560765
> /proton/trunk/proton-c/src/engine/engine.c 1560765
> /proton/trunk/proton-c/src/messenger/messenger.c 1560765
> /proton/trunk/proton-c/src/object/object.c 1560765
> /proton/trunk/proton-c/src/transport/transport.h 1560765
> /proton/trunk/proton-c/src/transport/transport.c 1560765
>
> Diff: https://reviews.apache.org/r/17329/diff/
>
>
> Testing
> -------
>
> Unit tests, including valgrind tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 17329: [proton-c] Manage references properly to allow
resource deallocation.
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17329/#review32832
-----------------------------------------------------------
/proton/trunk/proton-c/src/engine/engine.c
<https://reviews.apache.org/r/17329/#comment61839>
I'm putting this comment here because it kind of relates to changing the pn_free to a pn_decref, but really it's about the overall diff. Assuming I'm reading it correctly, I believe you have modified the ref counting semantics such that holding a pointer to a child object prevents the parent from being collected. While this makes sense from a python perspective, this actually makes the C API far more brittle since I can't simply free the connection and be sure that it and all of its components go away. I need to ensure that I track and free every single child object that I've created. This seems kind of annoying since the connection is tracking all that stuff anyways.
I thought the idea was that the connection level free blows away everything, but that the python binding keeps parent pointers to prevent the connection from being blown away.
That said, I don't think it's necessarily bad to implement these kind of semantics in C if it means less work in the bindings, however I think there should still be a connection level free that blows everything away if only for situations in C where you want to robustly clean up.
- Rafael Schloming
On Jan. 27, 2014, 1:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17329/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 1:33 p.m.)
>
>
> Review request for qpid, Gordon Sim and Rafael Schloming.
>
>
> Bugs: proton-489
> https://issues.apache.org/jira/browse/proton-489
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Prior to this fix, freeing an endpoint would leave stale pointers in referring objects. This fix makes use of the object reference functionality to ensure an endpoint is only removed when all references to it are dropped.
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/include/proton/object.h 1560765
> /proton/trunk/proton-c/src/engine/engine.c 1560765
> /proton/trunk/proton-c/src/messenger/messenger.c 1560765
> /proton/trunk/proton-c/src/object/object.c 1560765
> /proton/trunk/proton-c/src/transport/transport.h 1560765
> /proton/trunk/proton-c/src/transport/transport.c 1560765
>
> Diff: https://reviews.apache.org/r/17329/diff/
>
>
> Testing
> -------
>
> Unit tests, including valgrind tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 17329: [proton-c] Manage references properly to allow
resource deallocation.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17329/
-----------------------------------------------------------
(Updated Jan. 27, 2014, 1:33 p.m.)
Review request for qpid, Gordon Sim and Rafael Schloming.
Changes
-------
Do not deallocate endpoint while it is on the "modified" list.
Bugs: proton-489
https://issues.apache.org/jira/browse/proton-489
Repository: qpid
Description
-------
Prior to this fix, freeing an endpoint would leave stale pointers in referring objects. This fix makes use of the object reference functionality to ensure an endpoint is only removed when all references to it are dropped.
Diffs (updated)
-----
/proton/trunk/proton-c/include/proton/object.h 1560765
/proton/trunk/proton-c/src/engine/engine.c 1560765
/proton/trunk/proton-c/src/messenger/messenger.c 1560765
/proton/trunk/proton-c/src/object/object.c 1560765
/proton/trunk/proton-c/src/transport/transport.h 1560765
/proton/trunk/proton-c/src/transport/transport.c 1560765
Diff: https://reviews.apache.org/r/17329/diff/
Testing
-------
Unit tests, including valgrind tests.
Thanks,
Kenneth Giusti