qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From acon...@apache.org
Subject qpid-proton git commit: PROTON-1288: c++ fix caching bug in proton::map
Date Fri, 26 May 2017 02:37:57 GMT
Repository: qpid-proton
Updated Branches:
  refs/heads/master b86234c65 -> f66938024


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/master
Commit: f66938024bbb85036aa30ad42e014b912435cacd
Parents: b86234c
Author: Alan Conway <aconway@redhat.com>
Authored: Thu May 25 22:35:34 2017 -0400
Committer: Alan Conway <aconway@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-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message