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