You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by as...@apache.org on 2017/06/02 22:51:27 UTC
[13/20] qpid-proton git commit: PROTON-1288: c++ fix caching bug in
proton::map
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 <ac...@redhat.com>
Authored: Thu May 25 22:35:34 2017 -0400
Committer: Alan Conway <ac...@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