You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Cliff Jansen <cl...@gmail.com> on 2015/08/07 15:46:53 UTC
Review Request 37217: Switch from handle to concrete class or
reference in C++ client
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/
-----------------------------------------------------------
Review request for qpid, Alan Conway and Andrew Stitcher.
Bugs: PROTON-865
https://issues.apache.org/jira/browse/PROTON-865
Repository: qpid-proton-git
Description
-------
This is a mini refactor of a single class to see if the chosen direction for generally removing handle<foo> is sane.
I chose the connection class to try based on medium complexity and interaction with other moving parts (not visible to the user, the connector/override and life cycle issues).
The connection is now non copyable.
If the connection "self-created" (i.e. an inbound connection from a listener, arriving via a reactor event, or implicit connection from container.create_receiver), it is not "owned", and its life ends when the pn_connection_t dies (dies for good, i.e. when it releases its attachment records).
If the user creates the connection explicitly, it is "owned" and destructs as normal. It is unclear if the Python behavior should be preserved, where the connection lives on un-owned, or destruction means "all gone" with tear down of the transport and connector. I chose the latter as perhaps least surprise for the user.
The Proton C++ code can use either model internally to taste.
Perhaps the connection object should have a pin/unpin or incref/decref ability so that the user can hang onto it past the event delivery. As a hack, they can always do pn_incref(connection.pn_connection()) and decref it when done.
It is not clear to me that the counted class will ever be used separately from the context<foo> class, so they might get rolled together in the future. The memory management is accomplished by a special proton metaclass that is refcounted, but neither allocates nor frees memory.
connection::getZZZcontainer() is an unintended artifact of the refactor. I'm not sure why gcc is fussing over the original signature, but I don't wish to target its resolution in this review.
Diffs
-----
examples/cpp/helloworld.cpp 34e5076
examples/cpp/server.cpp 78b78d3
proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
proton-c/bindings/cpp/include/proton/connection.hpp af3c585
proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
proton-c/bindings/cpp/src/connection.cpp 2b335a4
proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
proton-c/bindings/cpp/src/connector.hpp ad373a9
proton-c/bindings/cpp/src/connector.cpp 559a7fd
proton-c/bindings/cpp/src/container.cpp a424c0b
proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
proton-c/bindings/cpp/src/container_impl.cpp e328251
proton-c/bindings/cpp/src/context.cpp PRE-CREATION
proton-c/bindings/cpp/src/contexts.cpp 98c502b
proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
proton-c/bindings/cpp/src/link.cpp 2cef0a2
proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
proton-c/bindings/cpp/src/session.cpp 5742f13
Diff: https://reviews.apache.org/r/37217/diff/
Testing
-------
passes ctest on rhel7
Thanks,
Cliff Jansen
Re: Review Request 37217: Switch from handle to concrete class or
reference in C++ client
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 12, 2015, 3:20 p.m., Alan Conway wrote:
> > I want to clarify my thoughts for discussion, I'm working on putting this into code.
> >
> > DESIGN PRINCIPLES:
> >
> > 1. C++ is not python. In C++ it is normal to provide "raw" alloc/free style pairs, esp. when wrapping C. Automation of memory management in C++ is done by a "smart pointer" layer (e.g. shared_ptr, unique_ptr) that is *separate* from the underlying raw access.
> >
> > 2. Proton refcounting should be considered an internal implementation detail of proton, NOT a feature that we use in the C++ binding. If need to refcount in the binding we should maintain our own refcounts to references *from the binding*, not rely on pn refcounts.
> >
> > 3. I suggest something like this: for pn_foo we have
> > a. foo : public wrapper<pn_foo> {} behaves like a plain pn_foo* but has convenience C++ member functions and "free()" member calling appropriate pn_foo_free()
> > b. unique<foo> : behaves like foo but calls foo.free() in dtor
> > c. shared<foo> : behaves like foo but maintains a refcount (like shared_ptr) and calls foo.free() on last ref.
> >
> > 4. Objects belonging to the binding itself that are not wrapper<pn_foo> should have normal new/delete semantics, and be manageable by std::shared_ptr, std::unique_ptr etc.
> >
> > Some justification for 2: Consider this from the public proton API doc for pn_link_target:
> >
> > * The pointer returned by this operation is valid until the link
> > * object is freed.
> >
> > Note the text specificaly says the pointer valid *until the link is freed*. It does *not* say you can incref the pointer to keep it around longer. It so happens that right now that actually works but IMO that is an implementation detail we should not rely on. In a future, simpler implementation of proton as a more C-like library that is not so heavily influenced by python it might NOT work. The python bindings rely on proton refcounts because they were specifically designed to support python, I don't think languages like C++ or Go should rely on them (and IMO this detail of python and ruby-like languages should not have been pushed into the C library, but should have been dealt with in those bindings) Go and C++ which have their own interface with C should treat proton as a simple C library.
> >
> > I suspect that simple 3a + 3b will cover most cases, I'm not sure we even need 3c but it isn't that hard to do.
> >
> > ASIDE: We could do some really freaky C++ magic and make wrapper<pn_foo> look exactly like a real C++ object with a real pointer (imaginary C++ objects occupying the same memory location as the pn_foo_*, casting their `this` pointers with overloaded op delete and all that fun stuff) but I think that would be, um, silly.
Here's some code to show what I mean, I created a new review from tip of cjansen-cpp-client as otherwise the diffs would quite confusing.
https://reviews.apache.org/r/37408/
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review95090
-----------------------------------------------------------
On Aug. 7, 2015, 1:46 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37217/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:46 p.m.)
>
>
> Review request for qpid, Alan Conway and Andrew Stitcher.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This is a mini refactor of a single class to see if the chosen direction for generally removing handle<foo> is sane.
>
>
> I chose the connection class to try based on medium complexity and interaction with other moving parts (not visible to the user, the connector/override and life cycle issues).
>
> The connection is now non copyable.
>
> If the connection "self-created" (i.e. an inbound connection from a listener, arriving via a reactor event, or implicit connection from container.create_receiver), it is not "owned", and its life ends when the pn_connection_t dies (dies for good, i.e. when it releases its attachment records).
>
> If the user creates the connection explicitly, it is "owned" and destructs as normal. It is unclear if the Python behavior should be preserved, where the connection lives on un-owned, or destruction means "all gone" with tear down of the transport and connector. I chose the latter as perhaps least surprise for the user.
>
> The Proton C++ code can use either model internally to taste.
>
> Perhaps the connection object should have a pin/unpin or incref/decref ability so that the user can hang onto it past the event delivery. As a hack, they can always do pn_incref(connection.pn_connection()) and decref it when done.
>
> It is not clear to me that the counted class will ever be used separately from the context<foo> class, so they might get rolled together in the future. The memory management is accomplished by a special proton metaclass that is refcounted, but neither allocates nor frees memory.
>
> connection::getZZZcontainer() is an unintended artifact of the refactor. I'm not sure why gcc is fussing over the original signature, but I don't wish to target its resolution in this review.
>
>
> Diffs
> -----
>
> examples/cpp/helloworld.cpp 34e5076
> examples/cpp/server.cpp 78b78d3
> proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
> proton-c/bindings/cpp/include/proton/connection.hpp af3c585
> proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
> proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
> proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
> proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
> proton-c/bindings/cpp/src/connection.cpp 2b335a4
> proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
> proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
> proton-c/bindings/cpp/src/connector.hpp ad373a9
> proton-c/bindings/cpp/src/connector.cpp 559a7fd
> proton-c/bindings/cpp/src/container.cpp a424c0b
> proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
> proton-c/bindings/cpp/src/container_impl.cpp e328251
> proton-c/bindings/cpp/src/context.cpp PRE-CREATION
> proton-c/bindings/cpp/src/contexts.cpp 98c502b
> proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
> proton-c/bindings/cpp/src/link.cpp 2cef0a2
> proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
> proton-c/bindings/cpp/src/session.cpp 5742f13
>
> Diff: https://reviews.apache.org/r/37217/diff/
>
>
> Testing
> -------
>
> passes ctest on rhel7
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 37217: Switch from handle to concrete class or
reference in C++ client
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review95090
-----------------------------------------------------------
I want to clarify my thoughts for discussion, I'm working on putting this into code.
DESIGN PRINCIPLES:
1. C++ is not python. In C++ it is normal to provide "raw" alloc/free style pairs, esp. when wrapping C. Automation of memory management in C++ is done by a "smart pointer" layer (e.g. shared_ptr, unique_ptr) that is *separate* from the underlying raw access.
2. Proton refcounting should be considered an internal implementation detail of proton, NOT a feature that we use in the C++ binding. If need to refcount in the binding we should maintain our own refcounts to references *from the binding*, not rely on pn refcounts.
3. I suggest something like this: for pn_foo we have
a. foo : public wrapper<pn_foo> {} behaves like a plain pn_foo* but has convenience C++ member functions and "free()" member calling appropriate pn_foo_free()
b. unique<foo> : behaves like foo but calls foo.free() in dtor
c. shared<foo> : behaves like foo but maintains a refcount (like shared_ptr) and calls foo.free() on last ref.
4. Objects belonging to the binding itself that are not wrapper<pn_foo> should have normal new/delete semantics, and be manageable by std::shared_ptr, std::unique_ptr etc.
Some justification for 2: Consider this from the public proton API doc for pn_link_target:
* The pointer returned by this operation is valid until the link
* object is freed.
Note the text specificaly says the pointer valid *until the link is freed*. It does *not* say you can incref the pointer to keep it around longer. It so happens that right now that actually works but IMO that is an implementation detail we should not rely on. In a future, simpler implementation of proton as a more C-like library that is not so heavily influenced by python it might NOT work. The python bindings rely on proton refcounts because they were specifically designed to support python, I don't think languages like C++ or Go should rely on them (and IMO this detail of python and ruby-like languages should not have been pushed into the C library, but should have been dealt with in those bindings) Go and C++ which have their own interface with C should treat proton as a simple C library.
I suspect that simple 3a + 3b will cover most cases, I'm not sure we even need 3c but it isn't that hard to do.
ASIDE: We could do some really freaky C++ magic and make wrapper<pn_foo> look exactly like a real C++ object with a real pointer (imaginary C++ objects occupying the same memory location as the pn_foo_*, casting their `this` pointers with overloaded op delete and all that fun stuff) but I think that would be, um, silly.
- Alan Conway
On Aug. 7, 2015, 1:46 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37217/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:46 p.m.)
>
>
> Review request for qpid, Alan Conway and Andrew Stitcher.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This is a mini refactor of a single class to see if the chosen direction for generally removing handle<foo> is sane.
>
>
> I chose the connection class to try based on medium complexity and interaction with other moving parts (not visible to the user, the connector/override and life cycle issues).
>
> The connection is now non copyable.
>
> If the connection "self-created" (i.e. an inbound connection from a listener, arriving via a reactor event, or implicit connection from container.create_receiver), it is not "owned", and its life ends when the pn_connection_t dies (dies for good, i.e. when it releases its attachment records).
>
> If the user creates the connection explicitly, it is "owned" and destructs as normal. It is unclear if the Python behavior should be preserved, where the connection lives on un-owned, or destruction means "all gone" with tear down of the transport and connector. I chose the latter as perhaps least surprise for the user.
>
> The Proton C++ code can use either model internally to taste.
>
> Perhaps the connection object should have a pin/unpin or incref/decref ability so that the user can hang onto it past the event delivery. As a hack, they can always do pn_incref(connection.pn_connection()) and decref it when done.
>
> It is not clear to me that the counted class will ever be used separately from the context<foo> class, so they might get rolled together in the future. The memory management is accomplished by a special proton metaclass that is refcounted, but neither allocates nor frees memory.
>
> connection::getZZZcontainer() is an unintended artifact of the refactor. I'm not sure why gcc is fussing over the original signature, but I don't wish to target its resolution in this review.
>
>
> Diffs
> -----
>
> examples/cpp/helloworld.cpp 34e5076
> examples/cpp/server.cpp 78b78d3
> proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
> proton-c/bindings/cpp/include/proton/connection.hpp af3c585
> proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
> proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
> proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
> proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
> proton-c/bindings/cpp/src/connection.cpp 2b335a4
> proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
> proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
> proton-c/bindings/cpp/src/connector.hpp ad373a9
> proton-c/bindings/cpp/src/connector.cpp 559a7fd
> proton-c/bindings/cpp/src/container.cpp a424c0b
> proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
> proton-c/bindings/cpp/src/container_impl.cpp e328251
> proton-c/bindings/cpp/src/context.cpp PRE-CREATION
> proton-c/bindings/cpp/src/contexts.cpp 98c502b
> proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
> proton-c/bindings/cpp/src/link.cpp 2cef0a2
> proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
> proton-c/bindings/cpp/src/session.cpp 5742f13
>
> Diff: https://reviews.apache.org/r/37217/diff/
>
>
> Testing
> -------
>
> passes ctest on rhel7
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 37217: Switch from handle to concrete class or
reference in C++ client
Posted by Cliff Jansen <cl...@gmail.com>.
> On Aug. 7, 2015, 2:54 p.m., Alan Conway wrote:
> > I would like to try something simpler (sorry!)
> >
> > There are 2 things I'd like to separate:
> >
> > 1. Clean, simple, no-value-add access to proton types.
> > 2. Separate convenience classes to make proton easier to use.
> >
> > For 1. the existing proton_handle<> template is perfect. I'd rename it proton_ptr<> to clarify it has simple pointer semantics. The proton_ptr subclasses just provide C++ method syntax for C calls and do some argument wrapping for strings, wrapping pointers as proton_ptr<> etc. This is what we already do for most types. We do no extra memory management at this layer, we just document the proton rules: pointers are valid inside event handlers, outside they are valid until the relevant _close events (including close events for containing objects) - if you break the rules, core dump. If you open()/create() it yourself, you must call close() when you are done or leak. Stick to simple C-style rules, IMO we should NOT expose proton's refcounting unless someone threatens to gouge our eyes out.
> >
> > For 2. e.g. proton::container: we can do what we like - a handle is not be unreasonable here if we can't make it simpler than that.
> >
> > My concern with proton::connection is it mixes the two. I would prefer a proton::connection that is nothing but a proton_ptr<pn_connection_t>, looking at connection_impl.hpp it isn't much more than that - I think we could probably strip the rest away, maybe just by adding a container* as a context or whatever it is called on the pn_connection_t.
> >
> > We do any memory management magic needed in the value-add layer, e.g. proton::container can have handlers to track connection lifecycles and ensure we don't delete the container while there are still connections referencing it. We can do this because we have access to all the events etc.
> >
> > Does this sound feasible? My main point here is to have a clean and complete proton layer with standard C-style "follow the rules or core dump/leak", and build ease-of-use and memory safety on top of that. If we mix them we will go insane. Proton's refcounting should be considered an implementation detail of the proton lib, not something we try to pull up into C++. It works for python because it was designed for python, C++ is not the same animal.
> >
> > This is what I did in Go and it works pretty well. My experience there is that if you want something like goroutine-safe access to proton, you really have to build it in terms of your lanugage concurrency rules. Working from simple pointer semantics and adding your own event handlers to manage lifecycle works fine. We could conceivably do a pthreads version of the go messaging API someday but not today :)
>
> Alan Conway wrote:
> I think proton::message is also an exception to the above, there's no useful wrapping of pn_message_t, so this is an ease-of-use class. We could debate whether it should have handle or value semantics but a hidden implementation is a good thing either way.
If you are happy to allow core dumps and leaks for breaking the rules, why not just give an actual pointer to the object. What additional benefit is supplied by proton_ptr<pn_connection_t> compared to *connection?
I don't take issue with the motivation, just asking from the point of view of providing the most C++ idiomatic API.
I take it from Andrew's comments in https://github.com/apache/qpid-proton/pull/35 that he is not fond of the PIMPL pattern for the C++ binding. There was some talk of performance, but I think it was also due to a preference for a different model from the API cleanliness/idiomatic-maximizing point of view.
FWIW, this patch (without a pin/unpin addition) works the way you suggest in terms of safety, i.e. the references are valid for as long as you proposed.
I agree that the C++ connection's internal organs are currently light, not requiring more context than already stored or storable in the pn_connection_t object. But in future, if there is some attempt to go parallel for performance, it seems to me that the connection object is likely to be the prime candidate for orchestrating things, so it might need some optimizations away from the pn_connection_t object. That was the main reason I treated it differently than e.g. link.
Note that this patch shows what moving away from the PIMPL pattern would look like. I am not personally advocating that it should be dropped.
- Cliff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review94542
-----------------------------------------------------------
On Aug. 7, 2015, 1:46 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37217/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:46 p.m.)
>
>
> Review request for qpid, Alan Conway and Andrew Stitcher.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This is a mini refactor of a single class to see if the chosen direction for generally removing handle<foo> is sane.
>
>
> I chose the connection class to try based on medium complexity and interaction with other moving parts (not visible to the user, the connector/override and life cycle issues).
>
> The connection is now non copyable.
>
> If the connection "self-created" (i.e. an inbound connection from a listener, arriving via a reactor event, or implicit connection from container.create_receiver), it is not "owned", and its life ends when the pn_connection_t dies (dies for good, i.e. when it releases its attachment records).
>
> If the user creates the connection explicitly, it is "owned" and destructs as normal. It is unclear if the Python behavior should be preserved, where the connection lives on un-owned, or destruction means "all gone" with tear down of the transport and connector. I chose the latter as perhaps least surprise for the user.
>
> The Proton C++ code can use either model internally to taste.
>
> Perhaps the connection object should have a pin/unpin or incref/decref ability so that the user can hang onto it past the event delivery. As a hack, they can always do pn_incref(connection.pn_connection()) and decref it when done.
>
> It is not clear to me that the counted class will ever be used separately from the context<foo> class, so they might get rolled together in the future. The memory management is accomplished by a special proton metaclass that is refcounted, but neither allocates nor frees memory.
>
> connection::getZZZcontainer() is an unintended artifact of the refactor. I'm not sure why gcc is fussing over the original signature, but I don't wish to target its resolution in this review.
>
>
> Diffs
> -----
>
> examples/cpp/helloworld.cpp 34e5076
> examples/cpp/server.cpp 78b78d3
> proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
> proton-c/bindings/cpp/include/proton/connection.hpp af3c585
> proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
> proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
> proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
> proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
> proton-c/bindings/cpp/src/connection.cpp 2b335a4
> proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
> proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
> proton-c/bindings/cpp/src/connector.hpp ad373a9
> proton-c/bindings/cpp/src/connector.cpp 559a7fd
> proton-c/bindings/cpp/src/container.cpp a424c0b
> proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
> proton-c/bindings/cpp/src/container_impl.cpp e328251
> proton-c/bindings/cpp/src/context.cpp PRE-CREATION
> proton-c/bindings/cpp/src/contexts.cpp 98c502b
> proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
> proton-c/bindings/cpp/src/link.cpp 2cef0a2
> proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
> proton-c/bindings/cpp/src/session.cpp 5742f13
>
> Diff: https://reviews.apache.org/r/37217/diff/
>
>
> Testing
> -------
>
> passes ctest on rhel7
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 37217: Switch from handle to concrete class or
reference in C++ client
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 7, 2015, 2:54 p.m., Alan Conway wrote:
> > I would like to try something simpler (sorry!)
> >
> > There are 2 things I'd like to separate:
> >
> > 1. Clean, simple, no-value-add access to proton types.
> > 2. Separate convenience classes to make proton easier to use.
> >
> > For 1. the existing proton_handle<> template is perfect. I'd rename it proton_ptr<> to clarify it has simple pointer semantics. The proton_ptr subclasses just provide C++ method syntax for C calls and do some argument wrapping for strings, wrapping pointers as proton_ptr<> etc. This is what we already do for most types. We do no extra memory management at this layer, we just document the proton rules: pointers are valid inside event handlers, outside they are valid until the relevant _close events (including close events for containing objects) - if you break the rules, core dump. If you open()/create() it yourself, you must call close() when you are done or leak. Stick to simple C-style rules, IMO we should NOT expose proton's refcounting unless someone threatens to gouge our eyes out.
> >
> > For 2. e.g. proton::container: we can do what we like - a handle is not be unreasonable here if we can't make it simpler than that.
> >
> > My concern with proton::connection is it mixes the two. I would prefer a proton::connection that is nothing but a proton_ptr<pn_connection_t>, looking at connection_impl.hpp it isn't much more than that - I think we could probably strip the rest away, maybe just by adding a container* as a context or whatever it is called on the pn_connection_t.
> >
> > We do any memory management magic needed in the value-add layer, e.g. proton::container can have handlers to track connection lifecycles and ensure we don't delete the container while there are still connections referencing it. We can do this because we have access to all the events etc.
> >
> > Does this sound feasible? My main point here is to have a clean and complete proton layer with standard C-style "follow the rules or core dump/leak", and build ease-of-use and memory safety on top of that. If we mix them we will go insane. Proton's refcounting should be considered an implementation detail of the proton lib, not something we try to pull up into C++. It works for python because it was designed for python, C++ is not the same animal.
> >
> > This is what I did in Go and it works pretty well. My experience there is that if you want something like goroutine-safe access to proton, you really have to build it in terms of your lanugage concurrency rules. Working from simple pointer semantics and adding your own event handlers to manage lifecycle works fine. We could conceivably do a pthreads version of the go messaging API someday but not today :)
I think proton::message is also an exception to the above, there's no useful wrapping of pn_message_t, so this is an ease-of-use class. We could debate whether it should have handle or value semantics but a hidden implementation is a good thing either way.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review94542
-----------------------------------------------------------
On Aug. 7, 2015, 1:46 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37217/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:46 p.m.)
>
>
> Review request for qpid, Alan Conway and Andrew Stitcher.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This is a mini refactor of a single class to see if the chosen direction for generally removing handle<foo> is sane.
>
>
> I chose the connection class to try based on medium complexity and interaction with other moving parts (not visible to the user, the connector/override and life cycle issues).
>
> The connection is now non copyable.
>
> If the connection "self-created" (i.e. an inbound connection from a listener, arriving via a reactor event, or implicit connection from container.create_receiver), it is not "owned", and its life ends when the pn_connection_t dies (dies for good, i.e. when it releases its attachment records).
>
> If the user creates the connection explicitly, it is "owned" and destructs as normal. It is unclear if the Python behavior should be preserved, where the connection lives on un-owned, or destruction means "all gone" with tear down of the transport and connector. I chose the latter as perhaps least surprise for the user.
>
> The Proton C++ code can use either model internally to taste.
>
> Perhaps the connection object should have a pin/unpin or incref/decref ability so that the user can hang onto it past the event delivery. As a hack, they can always do pn_incref(connection.pn_connection()) and decref it when done.
>
> It is not clear to me that the counted class will ever be used separately from the context<foo> class, so they might get rolled together in the future. The memory management is accomplished by a special proton metaclass that is refcounted, but neither allocates nor frees memory.
>
> connection::getZZZcontainer() is an unintended artifact of the refactor. I'm not sure why gcc is fussing over the original signature, but I don't wish to target its resolution in this review.
>
>
> Diffs
> -----
>
> examples/cpp/helloworld.cpp 34e5076
> examples/cpp/server.cpp 78b78d3
> proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
> proton-c/bindings/cpp/include/proton/connection.hpp af3c585
> proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
> proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
> proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
> proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
> proton-c/bindings/cpp/src/connection.cpp 2b335a4
> proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
> proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
> proton-c/bindings/cpp/src/connector.hpp ad373a9
> proton-c/bindings/cpp/src/connector.cpp 559a7fd
> proton-c/bindings/cpp/src/container.cpp a424c0b
> proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
> proton-c/bindings/cpp/src/container_impl.cpp e328251
> proton-c/bindings/cpp/src/context.cpp PRE-CREATION
> proton-c/bindings/cpp/src/contexts.cpp 98c502b
> proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
> proton-c/bindings/cpp/src/link.cpp 2cef0a2
> proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
> proton-c/bindings/cpp/src/session.cpp 5742f13
>
> Diff: https://reviews.apache.org/r/37217/diff/
>
>
> Testing
> -------
>
> passes ctest on rhel7
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 37217: Switch from handle to concrete class or
reference in C++ client
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 7, 2015, 2:54 p.m., Alan Conway wrote:
> > I would like to try something simpler (sorry!)
> >
> > There are 2 things I'd like to separate:
> >
> > 1. Clean, simple, no-value-add access to proton types.
> > 2. Separate convenience classes to make proton easier to use.
> >
> > For 1. the existing proton_handle<> template is perfect. I'd rename it proton_ptr<> to clarify it has simple pointer semantics. The proton_ptr subclasses just provide C++ method syntax for C calls and do some argument wrapping for strings, wrapping pointers as proton_ptr<> etc. This is what we already do for most types. We do no extra memory management at this layer, we just document the proton rules: pointers are valid inside event handlers, outside they are valid until the relevant _close events (including close events for containing objects) - if you break the rules, core dump. If you open()/create() it yourself, you must call close() when you are done or leak. Stick to simple C-style rules, IMO we should NOT expose proton's refcounting unless someone threatens to gouge our eyes out.
> >
> > For 2. e.g. proton::container: we can do what we like - a handle is not be unreasonable here if we can't make it simpler than that.
> >
> > My concern with proton::connection is it mixes the two. I would prefer a proton::connection that is nothing but a proton_ptr<pn_connection_t>, looking at connection_impl.hpp it isn't much more than that - I think we could probably strip the rest away, maybe just by adding a container* as a context or whatever it is called on the pn_connection_t.
> >
> > We do any memory management magic needed in the value-add layer, e.g. proton::container can have handlers to track connection lifecycles and ensure we don't delete the container while there are still connections referencing it. We can do this because we have access to all the events etc.
> >
> > Does this sound feasible? My main point here is to have a clean and complete proton layer with standard C-style "follow the rules or core dump/leak", and build ease-of-use and memory safety on top of that. If we mix them we will go insane. Proton's refcounting should be considered an implementation detail of the proton lib, not something we try to pull up into C++. It works for python because it was designed for python, C++ is not the same animal.
> >
> > This is what I did in Go and it works pretty well. My experience there is that if you want something like goroutine-safe access to proton, you really have to build it in terms of your lanugage concurrency rules. Working from simple pointer semantics and adding your own event handlers to manage lifecycle works fine. We could conceivably do a pthreads version of the go messaging API someday but not today :)
>
> Alan Conway wrote:
> I think proton::message is also an exception to the above, there's no useful wrapping of pn_message_t, so this is an ease-of-use class. We could debate whether it should have handle or value semantics but a hidden implementation is a good thing either way.
>
> Cliff Jansen wrote:
> If you are happy to allow core dumps and leaks for breaking the rules, why not just give an actual pointer to the object. What additional benefit is supplied by proton_ptr<pn_connection_t> compared to *connection?
>
> I don't take issue with the motivation, just asking from the point of view of providing the most C++ idiomatic API.
>
> I take it from Andrew's comments in https://github.com/apache/qpid-proton/pull/35 that he is not fond of the PIMPL pattern for the C++ binding. There was some talk of performance, but I think it was also due to a preference for a different model from the API cleanliness/idiomatic-maximizing point of view.
>
> FWIW, this patch (without a pin/unpin addition) works the way you suggest in terms of safety, i.e. the references are valid for as long as you proposed.
>
> I agree that the C++ connection's internal organs are currently light, not requiring more context than already stored or storable in the pn_connection_t object. But in future, if there is some attempt to go parallel for performance, it seems to me that the connection object is likely to be the prime candidate for orchestrating things, so it might need some optimizations away from the pn_connection_t object. That was the main reason I treated it differently than e.g. link.
>
> Note that this patch shows what moving away from the PIMPL pattern would look like. I am not personally advocating that it should be dropped.
Forget proton_ptr, I'm talking about the proton_handle here, renaming it was a silly idea. Also, to be clear I think the issue here is *not* "get rid of PIMPL", which is a legit pattern, but "provide low-overhead access to proton objects"
I'm not saying we don't do the memory management, I'm saying we separate it from the simple wrappers. We already have simple wrappers `foo : public proton_handle<pn_foo_t>` for most proton types that are nothing but C pointers with C++ member functions to give convenient access to `pn_foo_` functions. There are 2 exceptions: `connection` and (now thanks to me) `message`.
I like the idea of providing a "pure" set of simple wrappers for people who want to do something "raw" in C++ that is very close to the C api but don't want to be stuck calling C functions and translating std::string to `char*` by hand etc. That's what I did in Go - I have a complete set of raw wrappers for the pn_foo_t types (including connection and message) and a separate Pump to manage connection memory and Message to provide message-as-value semantics.
As for the remaining 2:
connection I *think* we can do the memory management for this via the container. I may be wrong as I haven't tried yet, we might need a per-connection class.
message: is almost a plain wrapper but it owns the pn_message_t and has value copy semantics.
So there are a couple of options:
- don't bother with separate "raw" wrappers for connection, the overhead is not that big a deal.
- have separate classes (need to figure out naming or namespaces) for "raw" and "value-add" versions of connection & message
- something else.
NOTE: I don't think we should expose proton refcounts in the C++ API, the above is based on that assumption. Unlike python and ruby it is perfectly normal in C++ for a container to own its contained objects and for pointers or references (or handles) to contained objects to be invalidated if the container is destroyed. I think pulling proton refcounts up into the C++ API will make things more complicated and error-prone not less. If we decide we do want to turn proton_handle into a fully ref-counted smart-pointer using proton refcount then we do have quite a bit more work to do.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review94542
-----------------------------------------------------------
On Aug. 7, 2015, 1:46 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37217/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:46 p.m.)
>
>
> Review request for qpid, Alan Conway and Andrew Stitcher.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This is a mini refactor of a single class to see if the chosen direction for generally removing handle<foo> is sane.
>
>
> I chose the connection class to try based on medium complexity and interaction with other moving parts (not visible to the user, the connector/override and life cycle issues).
>
> The connection is now non copyable.
>
> If the connection "self-created" (i.e. an inbound connection from a listener, arriving via a reactor event, or implicit connection from container.create_receiver), it is not "owned", and its life ends when the pn_connection_t dies (dies for good, i.e. when it releases its attachment records).
>
> If the user creates the connection explicitly, it is "owned" and destructs as normal. It is unclear if the Python behavior should be preserved, where the connection lives on un-owned, or destruction means "all gone" with tear down of the transport and connector. I chose the latter as perhaps least surprise for the user.
>
> The Proton C++ code can use either model internally to taste.
>
> Perhaps the connection object should have a pin/unpin or incref/decref ability so that the user can hang onto it past the event delivery. As a hack, they can always do pn_incref(connection.pn_connection()) and decref it when done.
>
> It is not clear to me that the counted class will ever be used separately from the context<foo> class, so they might get rolled together in the future. The memory management is accomplished by a special proton metaclass that is refcounted, but neither allocates nor frees memory.
>
> connection::getZZZcontainer() is an unintended artifact of the refactor. I'm not sure why gcc is fussing over the original signature, but I don't wish to target its resolution in this review.
>
>
> Diffs
> -----
>
> examples/cpp/helloworld.cpp 34e5076
> examples/cpp/server.cpp 78b78d3
> proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
> proton-c/bindings/cpp/include/proton/connection.hpp af3c585
> proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
> proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
> proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
> proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
> proton-c/bindings/cpp/src/connection.cpp 2b335a4
> proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
> proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
> proton-c/bindings/cpp/src/connector.hpp ad373a9
> proton-c/bindings/cpp/src/connector.cpp 559a7fd
> proton-c/bindings/cpp/src/container.cpp a424c0b
> proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
> proton-c/bindings/cpp/src/container_impl.cpp e328251
> proton-c/bindings/cpp/src/context.cpp PRE-CREATION
> proton-c/bindings/cpp/src/contexts.cpp 98c502b
> proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
> proton-c/bindings/cpp/src/link.cpp 2cef0a2
> proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
> proton-c/bindings/cpp/src/session.cpp 5742f13
>
> Diff: https://reviews.apache.org/r/37217/diff/
>
>
> Testing
> -------
>
> passes ctest on rhel7
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 37217: Switch from handle to concrete class or
reference in C++ client
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review94542
-----------------------------------------------------------
I would like to try something simpler (sorry!)
There are 2 things I'd like to separate:
1. Clean, simple, no-value-add access to proton types.
2. Separate convenience classes to make proton easier to use.
For 1. the existing proton_handle<> template is perfect. I'd rename it proton_ptr<> to clarify it has simple pointer semantics. The proton_ptr subclasses just provide C++ method syntax for C calls and do some argument wrapping for strings, wrapping pointers as proton_ptr<> etc. This is what we already do for most types. We do no extra memory management at this layer, we just document the proton rules: pointers are valid inside event handlers, outside they are valid until the relevant _close events (including close events for containing objects) - if you break the rules, core dump. If you open()/create() it yourself, you must call close() when you are done or leak. Stick to simple C-style rules, IMO we should NOT expose proton's refcounting unless someone threatens to gouge our eyes out.
For 2. e.g. proton::container: we can do what we like - a handle is not be unreasonable here if we can't make it simpler than that.
My concern with proton::connection is it mixes the two. I would prefer a proton::connection that is nothing but a proton_ptr<pn_connection_t>, looking at connection_impl.hpp it isn't much more than that - I think we could probably strip the rest away, maybe just by adding a container* as a context or whatever it is called on the pn_connection_t.
We do any memory management magic needed in the value-add layer, e.g. proton::container can have handlers to track connection lifecycles and ensure we don't delete the container while there are still connections referencing it. We can do this because we have access to all the events etc.
Does this sound feasible? My main point here is to have a clean and complete proton layer with standard C-style "follow the rules or core dump/leak", and build ease-of-use and memory safety on top of that. If we mix them we will go insane. Proton's refcounting should be considered an implementation detail of the proton lib, not something we try to pull up into C++. It works for python because it was designed for python, C++ is not the same animal.
This is what I did in Go and it works pretty well. My experience there is that if you want something like goroutine-safe access to proton, you really have to build it in terms of your lanugage concurrency rules. Working from simple pointer semantics and adding your own event handlers to manage lifecycle works fine. We could conceivably do a pthreads version of the go messaging API someday but not today :)
- Alan Conway
On Aug. 7, 2015, 1:46 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37217/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:46 p.m.)
>
>
> Review request for qpid, Alan Conway and Andrew Stitcher.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This is a mini refactor of a single class to see if the chosen direction for generally removing handle<foo> is sane.
>
>
> I chose the connection class to try based on medium complexity and interaction with other moving parts (not visible to the user, the connector/override and life cycle issues).
>
> The connection is now non copyable.
>
> If the connection "self-created" (i.e. an inbound connection from a listener, arriving via a reactor event, or implicit connection from container.create_receiver), it is not "owned", and its life ends when the pn_connection_t dies (dies for good, i.e. when it releases its attachment records).
>
> If the user creates the connection explicitly, it is "owned" and destructs as normal. It is unclear if the Python behavior should be preserved, where the connection lives on un-owned, or destruction means "all gone" with tear down of the transport and connector. I chose the latter as perhaps least surprise for the user.
>
> The Proton C++ code can use either model internally to taste.
>
> Perhaps the connection object should have a pin/unpin or incref/decref ability so that the user can hang onto it past the event delivery. As a hack, they can always do pn_incref(connection.pn_connection()) and decref it when done.
>
> It is not clear to me that the counted class will ever be used separately from the context<foo> class, so they might get rolled together in the future. The memory management is accomplished by a special proton metaclass that is refcounted, but neither allocates nor frees memory.
>
> connection::getZZZcontainer() is an unintended artifact of the refactor. I'm not sure why gcc is fussing over the original signature, but I don't wish to target its resolution in this review.
>
>
> Diffs
> -----
>
> examples/cpp/helloworld.cpp 34e5076
> examples/cpp/server.cpp 78b78d3
> proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
> proton-c/bindings/cpp/include/proton/connection.hpp af3c585
> proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
> proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
> proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
> proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
> proton-c/bindings/cpp/src/connection.cpp 2b335a4
> proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
> proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
> proton-c/bindings/cpp/src/connector.hpp ad373a9
> proton-c/bindings/cpp/src/connector.cpp 559a7fd
> proton-c/bindings/cpp/src/container.cpp a424c0b
> proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
> proton-c/bindings/cpp/src/container_impl.cpp e328251
> proton-c/bindings/cpp/src/context.cpp PRE-CREATION
> proton-c/bindings/cpp/src/contexts.cpp 98c502b
> proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
> proton-c/bindings/cpp/src/link.cpp 2cef0a2
> proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
> proton-c/bindings/cpp/src/session.cpp 5742f13
>
> Diff: https://reviews.apache.org/r/37217/diff/
>
>
> Testing
> -------
>
> passes ctest on rhel7
>
>
> Thanks,
>
> Cliff Jansen
>
>