From 2b87e942427bcac4109d08d0981da9f86122b894 Mon Sep 17 00:00:00 2001 From: dimitr Date: Fri, 19 Dec 2008 14:57:01 +0000 Subject: [PATCH] Use explicit iterators instead of the implicit (built-in) ones due to the thread-safety requirements. This change affects GenericMap only, as it's used globally in a few places and we have a proven crash there. I have a more complete solution (total cleanup of default accessors from BePlusTree and SparseBitmap) in my local tree (thanks to Dmitry Kovalenko), but it's incomplete (requires more changes inside the engine) and IMHO risky (~20 core modules are affected), so I'd like to defer finalization of this cleanup till v3.0. All objects of these types seem being protected by dbb_sync, so they shouldn't cause us any problems currently. --- src/common/classes/GenericMap.h | 90 ++++++++++++++++++++++++--------- src/jrd/DatabaseSnapshot.cpp | 8 +-- src/jrd/IntlManager.cpp | 6 +-- src/jrd/IntlUtil.cpp | 22 ++++---- src/jrd/cmp.cpp | 42 +++++++-------- src/jrd/jrd.cpp | 6 +-- src/jrd/jrd.h | 6 ++- src/jrd/met.epp | 4 +- src/jrd/unicode_util.cpp | 9 ++-- 9 files changed, 124 insertions(+), 69 deletions(-) diff --git a/src/common/classes/GenericMap.h b/src/common/classes/GenericMap.h index f8a9c8009c..4ebeafabe0 100644 --- a/src/common/classes/GenericMap.h +++ b/src/common/classes/GenericMap.h @@ -57,9 +57,32 @@ public: typedef typename KeyValuePair::first_type KeyType; typedef typename KeyValuePair::second_type ValueType; + typedef BePlusTree, KeyComparator> ValuesTree; + + class Accessor + { + public: + Accessor(GenericMap* map) : m_Accessor(&map->tree) {} + + KeyValuePair* current() const { return m_Accessor.current(); } + + bool getFirst() { return m_Accessor.getFirst(); } + bool getNext() { return m_Accessor.getNext(); } + + private: + Accessor(const Accessor&); + Accessor& operator=(const Accessor&); + + typename ValuesTree::Accessor m_Accessor; + }; + + friend class Accessor; + GenericMap() : tree(&getPool()), mCount(0) { } + explicit GenericMap(MemoryPool& a_pool) : AutoStorage(a_pool), tree(&getPool()), mCount(0) { } + ~GenericMap() { clear(); @@ -69,23 +92,29 @@ public: { clear(); - for (bool found = v.getFirst(); found; found = v.getNext()) + GenericMap::Accessor accessor(&v); + + for (bool found = accessor.getFirst(); found; found = accessor.getNext()) { - KeyValuePair* current_pair = v.current(); + const KeyValuePair* const current_pair = accessor.current(); put(current_pair->first, current_pair->second); } } void takeOwnership(GenericMap& from) { + fb_assert(this != &from); + clear(); tree = from.tree; mCount = from.mCount; - if (from.tree.getFirst()) { + ValuesTree::Accessor treeAccessor(&from.tree); + + if (treeAccessor.getFirst()) { while (true) { - bool haveMore = from.tree.fastRemove(); + bool haveMore = treeAccessor.fastRemove(); if (!haveMore) break; } @@ -97,10 +126,12 @@ public: // Clear the map void clear() { - if (tree.getFirst()) { + ValuesTree::Accessor treeAccessor(&tree); + + if (treeAccessor.getFirst()) { while (true) { - KeyValuePair* temp = tree.current(); - bool haveMore = tree.fastRemove(); + KeyValuePair* temp = treeAccessor.current(); + bool haveMore = treeAccessor.fastRemove(); delete temp; if (!haveMore) break; @@ -113,9 +144,11 @@ public: // Returns true if value existed bool remove(const KeyType& key) { - if (tree.locate(key)) { - KeyValuePair* var = tree.current(); - tree.fastRemove(); + ValuesTree::Accessor treeAccessor(&tree); + + if (treeAccessor.locate(key)) { + KeyValuePair* var = treeAccessor.current(); + treeAccessor.fastRemove(); delete var; mCount--; return true; @@ -127,8 +160,10 @@ public: // Returns true if value existed previously bool put(const KeyType& key, const ValueType& value) { - if (tree.locate(key)) { - tree.current()->second = value; + ValuesTree::Accessor treeAccessor(&tree); + + if (treeAccessor.locate(key)) { + treeAccessor.current()->second = value; return true; } @@ -141,8 +176,10 @@ public: // Returns pointer to the added empty value or null when key already exists ValueType* put(const KeyType& key) { - if (tree.locate(key)) { - return 0; + ValuesTree::Accessor treeAccessor(&tree); + + if (treeAccessor.locate(key)) { + return NULL; } KeyValuePair* var = FB_NEW(getPool()) KeyValuePair(getPool()); @@ -155,32 +192,35 @@ public: // Returns true if value is found bool get(const KeyType& key, ValueType& value) { - if (tree.locate(key)) { - value = tree.current()->second; + ValuesTree::Accessor treeAccessor(&tree); + + if (treeAccessor.locate(key)) { + value = treeAccessor.current()->second; return true; } return false; } - bool getFirst() { return tree.getFirst(); } + // Returns pointer to the found value or null otherwise + ValueType* get(const KeyType& key) + { + ValuesTree::Accessor treeAccessor(&tree); - bool getLast() { return tree.getLast(); } + if (treeAccessor.locate(key)) { + return &treeAccessor.current()->second; + } - bool getNext() { return tree.getNext(); } - - bool getPrev() { return tree.getPrev(); } - - KeyValuePair* current() const { return tree.current(); } + return NULL; + } bool exist(const KeyType& key) { - return tree.locate(key); + return ValuesTree::Accessor(&tree).locate(key); } size_t count() const { return mCount; } - typedef BePlusTree, KeyComparator> ValuesTree; private: ValuesTree tree; size_t mCount; diff --git a/src/jrd/DatabaseSnapshot.cpp b/src/jrd/DatabaseSnapshot.cpp index b3e3edaa69..38944668f3 100644 --- a/src/jrd/DatabaseSnapshot.cpp +++ b/src/jrd/DatabaseSnapshot.cpp @@ -1210,7 +1210,9 @@ void DatabaseSnapshot::putContextVars(Firebird::StringMap& variables, Firebird::ClumpletWriter& writer, int object_id, bool is_attachment) { - for (bool found = variables.getFirst(); found; found = variables.getNext()) + StringMap::Accessor accessor(&variables); + + for (bool found = accessor.getFirst(); found; found = accessor.getNext()) { writer.insertByte(TAG_RECORD, rel_mon_ctx_vars); @@ -1219,8 +1221,8 @@ void DatabaseSnapshot::putContextVars(Firebird::StringMap& variables, else writer.insertInt(f_mon_ctx_var_tra_id, object_id); - writer.insertString(f_mon_ctx_var_name, variables.current()->first); - writer.insertString(f_mon_ctx_var_value, variables.current()->second); + writer.insertString(f_mon_ctx_var_name, accessor.current()->first); + writer.insertString(f_mon_ctx_var_value, accessor.current()->second); } } diff --git a/src/jrd/IntlManager.cpp b/src/jrd/IntlManager.cpp index 487a19639d..bb1a61fa71 100644 --- a/src/jrd/IntlManager.cpp +++ b/src/jrd/IntlManager.cpp @@ -58,10 +58,10 @@ namespace ~ModulesMap() { // unload modules - for (bool found = getFirst(); found; found = getNext()) + Accessor accessor(this); + for (bool found = accessor.getFirst(); found; found = accessor.getNext()) { - ModuleLoader::Module* module = current()->second; - delete module; + delete accessor.current()->second; } } }; diff --git a/src/jrd/IntlUtil.cpp b/src/jrd/IntlUtil.cpp index 3e5c3629ea..76391fc472 100644 --- a/src/jrd/IntlUtil.cpp +++ b/src/jrd/IntlUtil.cpp @@ -77,7 +77,9 @@ static ULONG unicodeCanonical(texttype* tt, ULONG srcLen, const UCHAR* src, string IntlUtil::generateSpecificAttributes( Jrd::CharSet* cs, SpecificAttributesMap& map) { - bool found = map.getFirst(); + SpecificAttributesMap::Accessor accessor(&map); + + bool found = accessor.getFirst(); string s; while (found) @@ -85,7 +87,7 @@ string IntlUtil::generateSpecificAttributes( UCHAR c[sizeof(ULONG)]; ULONG size; - SpecificAttribute* attribute = map.current(); + SpecificAttribute* attribute = accessor.current(); s += escapeAttribute(cs, attribute->first); @@ -99,7 +101,7 @@ string IntlUtil::generateSpecificAttributes( s += escapeAttribute(cs, attribute->second); - found = map.getNext(); + found = accessor.getNext(); if (found) { @@ -420,7 +422,9 @@ bool IntlUtil::initUnicodeCollation(texttype* tt, charset* cs, const ASCII* name IntlUtil::SpecificAttributesMap map16; - bool found = map.getFirst(); + SpecificAttributesMap::Accessor accessor(&map); + + bool found = accessor.getFirst(); while (found) { @@ -429,20 +433,20 @@ bool IntlUtil::initUnicodeCollation(texttype* tt, charset* cs, const ASCII* name ULONG errPosition; s1.resize(cs->charset_to_unicode.csconvert_fn_convert( - &cs->charset_to_unicode, map.current()->first.length(), NULL, 0, NULL, &errCode, &errPosition)); + &cs->charset_to_unicode, accessor.current()->first.length(), NULL, 0, NULL, &errCode, &errPosition)); s1.resize(cs->charset_to_unicode.csconvert_fn_convert( - &cs->charset_to_unicode, map.current()->first.length(), (UCHAR*) map.current()->first.c_str(), + &cs->charset_to_unicode, accessor.current()->first.length(), (UCHAR*) accessor.current()->first.c_str(), s1.getCapacity(), s1.begin(), &errCode, &errPosition)); s2.resize(cs->charset_to_unicode.csconvert_fn_convert( - &cs->charset_to_unicode, map.current()->second.length(), NULL, 0, NULL, &errCode, &errPosition)); + &cs->charset_to_unicode, accessor.current()->second.length(), NULL, 0, NULL, &errCode, &errPosition)); s2.resize(cs->charset_to_unicode.csconvert_fn_convert( - &cs->charset_to_unicode, map.current()->second.length(), (UCHAR*) map.current()->second.c_str(), + &cs->charset_to_unicode, accessor.current()->second.length(), (UCHAR*) accessor.current()->second.c_str(), s2.getCapacity(), s2.begin(), &errCode, &errPosition)); map16.put(string((char*) s1.begin(), s1.getCount()), string((char*) s2.begin(), s2.getCount())); - found = map.getNext(); + found = accessor.getNext(); } UnicodeUtil::Utf16Collation* collation = diff --git a/src/jrd/cmp.cpp b/src/jrd/cmp.cpp index 4edfa9a53a..b0aa9ba7a9 100644 --- a/src/jrd/cmp.cpp +++ b/src/jrd/cmp.cpp @@ -2031,26 +2031,27 @@ jrd_req* CMP_make_request(thread_db* tdbb, CompilerScratch* csb, bool internal_f csb->csb_node = CMP_pass1(tdbb, csb, csb->csb_node); // Copy and compile (pass1) domains DEFAULT and constraints. - bool found = csb->csb_map_field_info.getFirst(); - while (found) { - FieldInfo& fieldInfo = csb->csb_map_field_info.current()->second; - UCHAR local_map[MAP_LENGTH]; + MapFieldInfo::Accessor accessor(&csb->csb_map_field_info); - AutoSetRestore autoRemapVariable(&csb->csb_remap_variable, - (csb->csb_variables ? csb->csb_variables->count() : 0) + 1); + for (bool found = accessor.getFirst(); found; found = accessor.getNext()) + { + FieldInfo& fieldInfo = accessor.current()->second; + UCHAR local_map[MAP_LENGTH]; - fieldInfo.defaultValue = copy(tdbb, csb, fieldInfo.defaultValue, local_map, 0, NULL, false); + AutoSetRestore autoRemapVariable(&csb->csb_remap_variable, + (csb->csb_variables ? csb->csb_variables->count() : 0) + 1); - csb->csb_remap_variable = (csb->csb_variables ? csb->csb_variables->count() : 0) + 1; + fieldInfo.defaultValue = copy(tdbb, csb, fieldInfo.defaultValue, local_map, 0, NULL, false); - fieldInfo.validation = copy(tdbb, csb, fieldInfo.validation, local_map, 0, NULL, false); + csb->csb_remap_variable = (csb->csb_variables ? csb->csb_variables->count() : 0) + 1; - fieldInfo.defaultValue = CMP_pass1(tdbb, csb, fieldInfo.defaultValue); - fieldInfo.validation = CMP_pass1(tdbb, csb, fieldInfo.validation); + fieldInfo.validation = copy(tdbb, csb, fieldInfo.validation, local_map, 0, NULL, false); - found = csb->csb_map_field_info.getNext(); - } + fieldInfo.defaultValue = CMP_pass1(tdbb, csb, fieldInfo.defaultValue); + fieldInfo.validation = CMP_pass1(tdbb, csb, fieldInfo.validation); + } + } // scope csb->csb_impure = REQ_SIZE + REQ_TAIL * MAX(csb->csb_n_stream, 1); csb->csb_exec_sta.clear(); @@ -2058,16 +2059,17 @@ jrd_req* CMP_make_request(thread_db* tdbb, CompilerScratch* csb, bool internal_f csb->csb_node = CMP_pass2(tdbb, csb, csb->csb_node, 0); // Compile (pass2) domains DEFAULT and constraints - found = csb->csb_map_field_info.getFirst(); - while (found) { - FieldInfo& fieldInfo = csb->csb_map_field_info.current()->second; + MapFieldInfo::Accessor accessor(&csb->csb_map_field_info); - fieldInfo.defaultValue = CMP_pass2(tdbb, csb, fieldInfo.defaultValue, 0); - fieldInfo.validation = CMP_pass2(tdbb, csb, fieldInfo.validation, 0); + for (bool found = accessor.getFirst(); found; found = accessor.getNext()) + { + FieldInfo& fieldInfo = accessor.current()->second; - found = csb->csb_map_field_info.getNext(); - } + fieldInfo.defaultValue = CMP_pass2(tdbb, csb, fieldInfo.defaultValue, 0); + fieldInfo.validation = CMP_pass2(tdbb, csb, fieldInfo.validation, 0); + } + } // scope if (csb->csb_impure > MAX_REQUEST_SIZE) { IBERROR(226); // msg 226 request size limit exceeded diff --git a/src/jrd/jrd.cpp b/src/jrd/jrd.cpp index c348551791..71be32c77b 100644 --- a/src/jrd/jrd.cpp +++ b/src/jrd/jrd.cpp @@ -4888,10 +4888,10 @@ static void release_attachment(thread_db* tdbb, Attachment* attachment, ISC_STAT if (attachment->att_temp_pg_lock) LCK_release(tdbb, attachment->att_temp_pg_lock); - for (bool getResult = attachment->att_dsql_cache.getFirst(); getResult; - getResult = attachment->att_dsql_cache.getNext()) + DSqlCache::Accessor accessor(&attachment->att_dsql_cache); + for (bool getResult = accessor.getFirst(); getResult; getResult = accessor.getNext()) { - LCK_release(tdbb, attachment->att_dsql_cache.current()->second.lock); + LCK_release(tdbb, accessor.current()->second.lock); } #endif diff --git a/src/jrd/jrd.h b/src/jrd/jrd.h index e5126bc4aa..5a6dbe69ac 100644 --- a/src/jrd/jrd.h +++ b/src/jrd/jrd.h @@ -228,6 +228,9 @@ struct DSqlCacheItem bool obsolete; }; +typedef Firebird::GenericMap > > DSqlCache; + // // the attachment block; one is created for each attachment to a database @@ -330,8 +333,7 @@ public: #ifndef SUPERSERVER Lock* att_temp_pg_lock; // temporary pagespace ID lock #endif - Firebird::GenericMap > > att_dsql_cache; // DSQL cache locks + DSqlCache att_dsql_cache; // DSQL cache locks Firebird::SortedArray att_udf_pointers; dsql_dbb* att_dsql_instance; Firebird::Mutex att_mutex; // attachment mutex diff --git a/src/jrd/met.epp b/src/jrd/met.epp index 02e1557a9d..32ab00b078 100644 --- a/src/jrd/met.epp +++ b/src/jrd/met.epp @@ -4192,7 +4192,9 @@ static DSqlCacheItem* get_dsql_cache_item(thread_db* tdbb, int type, const Fireb memcpy(item->lock->lck_key.lck_string, key.c_str(), key.length()); } else - item = &attachment->att_dsql_cache.current()->second; + { + item = attachment->att_dsql_cache.get(key); + } return item; } diff --git a/src/jrd/unicode_util.cpp b/src/jrd/unicode_util.cpp index 59042415b1..1fcbdbde65 100644 --- a/src/jrd/unicode_util.cpp +++ b/src/jrd/unicode_util.cpp @@ -117,6 +117,8 @@ public: // cache ICU module instances to not load and unload many times class UnicodeUtil::ICUModules { + typedef GenericMap > > ModulesMap; + public: explicit ICUModules(MemoryPool&) { @@ -124,11 +126,12 @@ public: ~ICUModules() { - for (bool found = modules().getFirst(); found; found = modules().getNext()) - delete modules().current()->second; + ModulesMap::Accessor modulesAccessor(&modules()); + for (bool found = modulesAccessor.getFirst(); found; found = modulesAccessor.getNext()) + delete modulesAccessor.current()->second; } - InitInstance > > > modules; + InitInstance modules; RWLock lock; };