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