PROTON-1288: c++ fix caching bug in proton::map Discovered using quiver tests: `quiver foo --impl=qpid-proton-cpp`
map::clear() did not invalidate the cache as required by message::encode() to ensure that newly-decoded map values updated the caches correctly. Simplified the cache logic. Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/f6693802 Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/f6693802 Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/f6693802 Branch: refs/heads/PROTON-1488 Commit: f66938024bbb85036aa30ad42e014b912435cacd Parents: b86234c Author: Alan Conway <acon...@redhat.com> Authored: Thu May 25 22:35:34 2017 -0400 Committer: Alan Conway <acon...@redhat.com> Committed: Thu May 25 22:35:34 2017 -0400 ---------------------------------------------------------------------- proton-c/bindings/cpp/include/proton/map.hpp | 4 +- proton-c/bindings/cpp/include/proton/sender.hpp | 1 + .../bindings/cpp/src/connection_driver_test.cpp | 29 ++++++- proton-c/bindings/cpp/src/map.cpp | 83 +++++++------------- proton-c/bindings/cpp/src/message_test.cpp | 13 +++ 5 files changed, 70 insertions(+), 60 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6693802/proton-c/bindings/cpp/include/proton/map.hpp ---------------------------------------------------------------------- diff --git a/proton-c/bindings/cpp/include/proton/map.hpp b/proton-c/bindings/cpp/include/proton/map.hpp index 9798fad..86e4de7 100644 --- a/proton-c/bindings/cpp/include/proton/map.hpp +++ b/proton-c/bindings/cpp/include/proton/map.hpp @@ -115,9 +115,7 @@ class PN_CPP_CLASS_EXTERN map { mutable internal::pn_unique_ptr<map_type> map_; mutable proton::value value_; - void ensure() const; - const map_type& cache() const; - map_type& cache_update(); + map_type& cache() const; proton::value& flush() const; friend PN_CPP_EXTERN proton::codec::decoder& operator>> <>(proton::codec::decoder&, map&); http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6693802/proton-c/bindings/cpp/include/proton/sender.hpp ---------------------------------------------------------------------- diff --git a/proton-c/bindings/cpp/include/proton/sender.hpp b/proton-c/bindings/cpp/include/proton/sender.hpp index 8979bb4..f8c1e66 100644 --- a/proton-c/bindings/cpp/include/proton/sender.hpp +++ b/proton-c/bindings/cpp/include/proton/sender.hpp @@ -25,6 +25,7 @@ #include "./fwd.hpp" #include "./internal/export.hpp" #include "./link.hpp" +#include "./tracker.hpp" struct pn_link_t; struct pn_session_t; http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6693802/proton-c/bindings/cpp/src/connection_driver_test.cpp ---------------------------------------------------------------------- diff --git a/proton-c/bindings/cpp/src/connection_driver_test.cpp b/proton-c/bindings/cpp/src/connection_driver_test.cpp index f179601..a5771f9 100644 --- a/proton-c/bindings/cpp/src/connection_driver_test.cpp +++ b/proton-c/bindings/cpp/src/connection_driver_test.cpp @@ -25,8 +25,10 @@ #include "proton/io/connection_driver.hpp" #include "proton/io/link_namer.hpp" #include "proton/link.hpp" +#include "proton/message.hpp" #include "proton/messaging_handler.hpp" #include "proton/receiver_options.hpp" +#include "proton/sender.hpp" #include "proton/sender_options.hpp" #include "proton/source_options.hpp" #include "proton/types_fwd.hpp" @@ -123,6 +125,7 @@ struct record_handler : public messaging_handler { std::deque<proton::sender> senders; std::deque<proton::session> sessions; std::deque<std::string> unhandled_errors, transport_errors, connection_errors; + std::deque<proton::message> messages; void on_receiver_open(receiver &l) PN_CPP_OVERRIDE { receivers.push_back(l); @@ -147,6 +150,10 @@ struct record_handler : public messaging_handler { void on_error(const proton::error_condition& c) PN_CPP_OVERRIDE { unhandled_errors.push_back(c.what()); } + + void on_message(proton::delivery&, proton::message& m) PN_CPP_OVERRIDE { + messages.push_back(m); + } }; struct namer : public io::link_namer { @@ -307,16 +314,36 @@ void test_link_filters() { ASSERT_EQUAL(value("xxx"), bx.source().filters().get("xx")); } +void test_message() { + // Verify a message arrives intact + record_handler ha, hb; + driver_pair d(ha, hb); + + proton::sender s = d.a.connection().open_sender("x"); + proton::message m("barefoot"); + m.properties().put("x", "y"); + m.message_annotations().put("a", "b"); + s.send(m); + + while (hb.messages.size() == 0) + d.process(); + + proton::message m2 = quick_pop(hb.messages); + ASSERT_EQUAL(value("barefoot"), m2.body()); + ASSERT_EQUAL(value("y"), m2.properties().get("x")); + ASSERT_EQUAL(value("b"), m2.message_annotations().get("a")); +} } int main(int argc, char** argv) { int failed = 0; - RUN_ARGV_TEST(failed, test_link_filters()); RUN_ARGV_TEST(failed, test_driver_link_id()); RUN_ARGV_TEST(failed, test_endpoint_close()); RUN_ARGV_TEST(failed, test_driver_disconnected()); RUN_ARGV_TEST(failed, test_no_container()); RUN_ARGV_TEST(failed, test_spin_interrupt()); + RUN_ARGV_TEST(failed, test_message()); + RUN_ARGV_TEST(failed, test_link_filters()); return failed; } http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6693802/proton-c/bindings/cpp/src/map.cpp ---------------------------------------------------------------------- diff --git a/proton-c/bindings/cpp/src/map.cpp b/proton-c/bindings/cpp/src/map.cpp index 6fc80fa..29652d2 100644 --- a/proton-c/bindings/cpp/src/map.cpp +++ b/proton-c/bindings/cpp/src/map.cpp @@ -32,14 +32,9 @@ #include <string> // IMPLEMENTATION NOTES: -// - either value_, map_ or both can hold the up-to-date data -// - avoid encoding or decoding between map_ and value_ unless necessary -// - if (map.get()) then map_ is up to date -// - if (!value_.empty()) then value_ is up to date -// - if (map.get_() && !value_.empty()) then map_ and value_ have equivalent data -// - if (!map.get_() && value_.empty()) then that's equivalent to an empty map +// - if (map_.get()) then *map_ is the authority // - cache() ensures that *map_ is up to date -// - flush() ensures value_ is up to date +// - flush() ensures value_ is up to date and map_ is reset namespace proton { @@ -64,24 +59,11 @@ PN_CPP_EXTERN void swap(map<K,T>& x, map<K,T>& y) { } template <class K, class T> -void map<K,T>::ensure() const { - if (!map_) { - map_.reset(new map<K,T>::map_type); - } -} - -template <class K, class T> map<K,T>& map<K,T>::operator=(const map& x) { if (&x != this) { - if (!x.value_.empty()) { - map_.reset(); + map_.reset(x.map_.get() ? new map_type(*x.map_) : 0); + if (!map_) { value_ = x.value_; - } else if (x.map_.get()) { - value_.clear(); - ensure(); - *map_ = *x.map_; - } else { - clear(); } } return *this; @@ -107,41 +89,38 @@ map<K,T>::~map() {} // Make sure map_ is valid template <class K, class T> -const typename map<K,T>::map_type& map<K,T>::cache() const { +typename map<K,T>::map_type& map<K,T>::cache() const { if (!map_) { - ensure(); + map_.reset(new map_type); if (!value_.empty()) { - proton::get(value_, *map_); + try { + proton::get(value_, *map_); + } catch (...) { // Invalid value for the map, throw it away. + map_.reset(); + value_.clear(); + throw; + } } } return *map_; } -// Make sure map_ is valid, and mark value_ invalid -template <class K, class T> -typename map<K,T>::map_type& map<K,T>::cache_update() { - cache(); - value_.clear(); - return *map_; -} - template <class K, class T> value& map<K,T>::flush() const { - if (value_.empty()) { - // Create an empty map if need be, value_ must hold a valid map (even if empty) - // it must not be an empty (NULL_TYPE) proton::value. - ensure(); + if (map_.get()) { value_ = *map_; + map_.reset(); + } else if (value_.empty()) { + // Must contain an empty map, not be an empty value. + codec::encoder(value_) << codec::start::map() << codec::finish(); } return value_; } template <class K, class T> void map<K,T>::value(const proton::value& x) { - value_.clear(); - // Validate the value by decoding it into map_, throw if not a valid map value. - ensure(); - proton::get(x, *map_); + value_ = x; + cache(); // Validate the value by decoding to cache. } template <class K, class T> @@ -160,7 +139,7 @@ T map<K,T>::get(const K& k) const { template <class K, class T> void map<K,T>::put(const K& k, const T& v) { - cache_update()[k] = v; + cache()[k] = v; } template <class K, class T> @@ -168,7 +147,7 @@ size_t map<K,T>::erase(const K& k) { if (this->empty()) { return 0; } else { - return cache_update().erase(k); + return cache().erase(k); } } @@ -184,9 +163,7 @@ size_t map<K,T>::size() const { template <class K, class T> void map<K,T>::clear() { - if (map_.get()) { - map_->clear(); - } + map_.reset(); // Must invalidate the cache on clear() value_.clear(); } @@ -214,22 +191,16 @@ void map<K,T>::reset(pn_data_t *d) { template <class K, class T> PN_CPP_EXTERN proton::codec::decoder& operator>>(proton::codec::decoder& d, map<K,T>& m) { - // Decode to m.map_ rather than m.value_ to verify the data is of valid type. - m.value_.clear(); - m.ensure(); - d >> *m.map_; + m.map_.reset(); + d >> m.value_; + m.cache(); // Validate the value return d; } template <class K, class T> PN_CPP_EXTERN proton::codec::encoder& operator<<(proton::codec::encoder& e, const map<K,T>& m) { - if (!m.value_.empty()) { - return e << m.value_; // Copy the value - } - // Encode the (possibly empty) map_. - m.ensure(); - return e << *(m.map_); + return e << m.value(); // Copy the value } // Force the necessary template instantiations so that the library exports the correct symbols http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6693802/proton-c/bindings/cpp/src/message_test.cpp ---------------------------------------------------------------------- diff --git a/proton-c/bindings/cpp/src/message_test.cpp b/proton-c/bindings/cpp/src/message_test.cpp index eafea2e..0d7229f 100644 --- a/proton-c/bindings/cpp/src/message_test.cpp +++ b/proton-c/bindings/cpp/src/message_test.cpp @@ -167,6 +167,18 @@ void test_message_maps() { ASSERT(m3.message_annotations().empty()); } +void test_message_reuse() { + message m1("one"); + m1.properties().put("x", "y"); + + message m2("two"); + m2.properties().put("a", "b"); + + m1.decode(m2.encode()); // Use m1 for a newly decoded message + ASSERT_EQUAL(value("two"), m1.body()); + ASSERT_EQUAL(value("b"), m1.properties().get("a")); +} + } int main(int, char**) { @@ -175,5 +187,6 @@ int main(int, char**) { RUN_TEST(failed, test_message_defaults()); RUN_TEST(failed, test_message_body()); RUN_TEST(failed, test_message_maps()); + RUN_TEST(failed, test_message_reuse()); return failed; } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org For additional commands, e-mail: commits-h...@qpid.apache.org
