From c1e75de882bf7e9eb669f2d6b8c23330b7c3313e Mon Sep 17 00:00:00 2001 From: Scott Banachowski Date: Thu, 8 Oct 2009 00:12:44 +0000 Subject: [PATCH] AVRO-132. Fix multi-threading race condition when threads share schema objects. git-svn-id: https://svn.apache.org/repos/asf/hadoop/avro/trunk@822968 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES.txt | 3 +++ src/c++/api/Boost.hh | 4 ++-- src/c++/api/Node.hh | 27 +++++---------------------- src/c++/api/ValidSchema.hh | 4 ++-- src/c++/api/ValidatingReader.hh | 2 +- src/c++/api/Validator.hh | 9 +++++++-- src/c++/impl/NodeImpl.cc | 16 ++++++++-------- src/c++/impl/ValidSchema.cc | 6 +++--- src/c++/impl/ValidatingReader.cc | 2 +- src/c++/impl/ValidatingWriter.cc | 2 +- src/c++/impl/Validator.cc | 2 +- 11 files changed, 34 insertions(+), 43 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 9bfeb2a575a..30cce697ca2 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -70,6 +70,9 @@ Trunk (unreleased changes) BUG FIXES + AVRO-132. Fix multi-threading race condition when threads share schema objects. + (sbanacho) + AVRO-113. Fix endian bug with C++ integer/long varint codec. (Scott Banachowski via cutting) diff --git a/src/c++/api/Boost.hh b/src/c++/api/Boost.hh index 336d21861d7..a8ae1512425 100644 --- a/src/c++/api/Boost.hh +++ b/src/c++/api/Boost.hh @@ -55,7 +55,7 @@ namespace boost { typedef integral_constant true_type; typedef integral_constant false_type; -} +} // namespace boost #else #include #endif // AVRO_BOOST_NO_TRAIT @@ -93,7 +93,7 @@ namespace boost { private: std::vector ptrs_; }; -} +} // namespace boost #else #include #endif // AVRO_BOOST_NO_PTRVECTOR diff --git a/src/c++/api/Node.hh b/src/c++/api/Node.hh index 28fba162fe1..96641eede41 100644 --- a/src/c++/api/Node.hh +++ b/src/c++/api/Node.hh @@ -21,7 +21,7 @@ #include #include -#include +#include #include "Exception.hh" #include "Types.hh" @@ -30,7 +30,7 @@ namespace avro { class Node; -typedef boost::intrusive_ptr NodePtr; +typedef boost::shared_ptr NodePtr; /// Node is the building block for parse trees. Each node represents an avro @@ -40,10 +40,9 @@ typedef boost::intrusive_ptr NodePtr; /// The user does not use the Node object directly, they interface with Schema /// objects. /// -/// The Node object has an embedded reference count for use in the -/// boost::intrusive_ptr smart pointer. This is so that schemas may be reused -/// in other other schemas, without needing to worry about memory deallocation -/// for nodes that are added to multiple schema parse trees. +/// The Node object uses reference-counted pointers. This is so that schemas +/// may be reused in other other schemas, without needing to worry about memory +/// deallocation for nodes that are added to multiple schema parse trees. /// /// Node has minimal implementation, serving as an abstract base class for /// different node types. @@ -130,27 +129,11 @@ class Node : private boost::noncopyable private: - friend void intrusive_ptr_add_ref(Node *node); - friend void intrusive_ptr_release(Node *node); - const Type type_; int refCount_; bool locked_; }; -inline void -intrusive_ptr_add_ref(Node *node) { - ++(node->refCount_); -} - -inline void -intrusive_ptr_release(Node *node) { - --(node->refCount_); - if(node->refCount_ == 0) { - delete node; - } -} - } // namespace avro #endif diff --git a/src/c++/api/ValidSchema.hh b/src/c++/api/ValidSchema.hh index 045087f1275..689dc39237e 100644 --- a/src/c++/api/ValidSchema.hh +++ b/src/c++/api/ValidSchema.hh @@ -52,9 +52,9 @@ class ValidSchema : private boost::noncopyable return node_; } - void toJson(std::ostream &os); + void toJson(std::ostream &os) const; - void toFlatList(std::ostream &os); + void toFlatList(std::ostream &os) const; NodePtr followSymbol(const std::string &name) const { return symbolMap_.locateSymbol(name); diff --git a/src/c++/api/ValidatingReader.hh b/src/c++/api/ValidatingReader.hh index 88b5053ffa8..9eda3448e32 100644 --- a/src/c++/api/ValidatingReader.hh +++ b/src/c++/api/ValidatingReader.hh @@ -47,7 +47,7 @@ class ValidatingReader : private boost::noncopyable public: - explicit ValidatingReader(const ValidSchema &schema, InputStreamer &in); + ValidatingReader(const ValidSchema &schema, InputStreamer &in); template void getValue(T &val) { diff --git a/src/c++/api/Validator.hh b/src/c++/api/Validator.hh index de5a611bd76..63ecdc890b4 100644 --- a/src/c++/api/Validator.hh +++ b/src/c++/api/Validator.hh @@ -42,7 +42,7 @@ class Validator : private boost::noncopyable public: - Validator(const ValidSchema &schema); + explicit Validator(const ValidSchema &schema); void advance(); void advanceWithCount(int64_t val); @@ -80,7 +80,12 @@ class Validator : private boost::noncopyable void setupFlag(Type type); const ValidSchema &schema_; - NodePtr parseTree_; + + // since this only keeps a reference to the schema, to ensure its parse + // tree is not deleted, keep a copy of a shared pointer to the root of the + // tree + + const NodePtr parseTree_; Type nextType_; flag_t expectedTypesFlag_; diff --git a/src/c++/impl/NodeImpl.cc b/src/c++/impl/NodeImpl.cc index 66a13b0147e..9d050a777ce 100644 --- a/src/c++/impl/NodeImpl.cc +++ b/src/c++/impl/NodeImpl.cc @@ -159,36 +159,36 @@ nodeFromCompilerNode(CompilerNode &node) switch(node.type()) { case AVRO_ARRAY: - ptr = ( new NodeArray(node)); + ptr.reset ( new NodeArray(node)); break; case AVRO_ENUM: - ptr = ( new NodeEnum(node)); + ptr.reset ( new NodeEnum(node)); break; case AVRO_FIXED: - ptr = ( new NodeFixed(node)); + ptr.reset ( new NodeFixed(node)); break; case AVRO_MAP: - ptr = ( new NodeMap(node)); + ptr.reset ( new NodeMap(node)); break; case AVRO_RECORD: - ptr = ( new NodeRecord(node)); + ptr.reset ( new NodeRecord(node)); break; case AVRO_UNION: - ptr = ( new NodeUnion(node)); + ptr.reset ( new NodeUnion(node)); break; case AVRO_SYMBOLIC: - ptr = ( new NodeSymbolic(node)); + ptr.reset ( new NodeSymbolic(node)); break; default: if(isPrimitive(node.type())) { - ptr = ( new NodePrimitive(node.type())); + ptr.reset ( new NodePrimitive(node.type())); } else { throw Exception("Unknown type in nodeFromCompilerNode"); diff --git a/src/c++/impl/ValidSchema.cc b/src/c++/impl/ValidSchema.cc index a7eb04ac358..3e860993853 100644 --- a/src/c++/impl/ValidSchema.cc +++ b/src/c++/impl/ValidSchema.cc @@ -46,7 +46,7 @@ bool ValidSchema::validate(const NodePtr &node) { if(!node) { - node_ = new NodePrimitive(AVRO_NULL); + node_.reset(new NodePrimitive(AVRO_NULL)); } if(!node->isValid()) { @@ -78,14 +78,14 @@ ValidSchema::validate(const NodePtr &node) } void -ValidSchema::toJson(std::ostream &os) +ValidSchema::toJson(std::ostream &os) const { node_->printJson(os, 0); os << '\n'; } void -ValidSchema::toFlatList(std::ostream &os) +ValidSchema::toFlatList(std::ostream &os) const { node_->printBasicInfo(os); } diff --git a/src/c++/impl/ValidatingReader.cc b/src/c++/impl/ValidatingReader.cc index 89131c1ded1..0c621764793 100644 --- a/src/c++/impl/ValidatingReader.cc +++ b/src/c++/impl/ValidatingReader.cc @@ -78,4 +78,4 @@ ValidatingReader::getArrayBlockSize() return getCount(); } -} // namepspace avro +} // namespace avro diff --git a/src/c++/impl/ValidatingWriter.cc b/src/c++/impl/ValidatingWriter.cc index 4a0b9bf9a60..6af616978e1 100644 --- a/src/c++/impl/ValidatingWriter.cc +++ b/src/c++/impl/ValidatingWriter.cc @@ -98,4 +98,4 @@ ValidatingWriter::beginEnum(int64_t choice) } -} // namepspace avro +} // namespace avro diff --git a/src/c++/impl/Validator.cc b/src/c++/impl/Validator.cc index 3485c7d4158..744890598ce 100644 --- a/src/c++/impl/Validator.cc +++ b/src/c++/impl/Validator.cc @@ -295,4 +295,4 @@ Validator::getNextFieldName(std::string &name) const return found; } -} // namepspace avro +} // namespace avro