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