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