From 9858d0efedccfc7f3f4427206e4b62d68d0a0e05 Mon Sep 17 00:00:00 2001
From: scott snyder <sss@karma>
Date: Wed, 7 Oct 2020 13:16:10 -0400
Subject: [PATCH] CollectionBase: Enable thread-safety checking.
Enable thread-safety checking.
Fix resulting warnings.
CollectionFactory / CollectionService marked as not thread-safe
due to const-correctness violations.
---
Database/APR/CollectionBase/CMakeLists.txt | 2 +-
.../CollectionBase/ATLAS_CHECK_THREAD_SAFETY | 1 +
.../CollectionBase/CollectionDescription.h | 13 ++--
.../CollectionBase/CollectionFactory.h | 6 +-
.../CollectionBase/CollectionService.h | 5 +-
.../CollectionBase/ICollectionMetadata.h | 25 ++------
.../CollectionBase/ICollectionService.h | 5 +-
.../src/CollectionDescription.cpp | 60 +++++++++++++++++--
.../CollectionBase/src/CollectionFactory.cpp | 4 +-
Database/APR/CollectionBase/src/TokenList.cpp | 21 ++++++-
.../APR/CollectionBase/test/Factory_test.cxx | 6 +-
11 files changed, 106 insertions(+), 42 deletions(-)
create mode 100644 Database/APR/CollectionBase/CollectionBase/ATLAS_CHECK_THREAD_SAFETY
diff --git a/Database/APR/CollectionBase/CMakeLists.txt b/Database/APR/CollectionBase/CMakeLists.txt
index fc861d1f9f4..346d4cd3e1a 100644
--- a/Database/APR/CollectionBase/CMakeLists.txt
+++ b/Database/APR/CollectionBase/CMakeLists.txt
@@ -12,7 +12,7 @@ atlas_add_library( CollectionBase
src/*.cpp
PUBLIC_HEADERS CollectionBase
INCLUDE_DIRS ${Boost_INCLUDE_DIRS} ${CORAL_INCLUDE_DIRS}
- LINK_LIBRARIES ${Boost_LIBRARIES} ${CORAL_LIBRARIES} FileCatalog
+ LINK_LIBRARIES ${Boost_LIBRARIES} ${CORAL_LIBRARIES} FileCatalog CxxUtils
PRIVATE_LINK_LIBRARIES POOLCore PersistentDataModel )
atlas_add_dictionary( CollectionDict
diff --git a/Database/APR/CollectionBase/CollectionBase/ATLAS_CHECK_THREAD_SAFETY b/Database/APR/CollectionBase/CollectionBase/ATLAS_CHECK_THREAD_SAFETY
new file mode 100644
index 00000000000..51c5d7f4f8a
--- /dev/null
+++ b/Database/APR/CollectionBase/CollectionBase/ATLAS_CHECK_THREAD_SAFETY
@@ -0,0 +1 @@
+Database/APR/CollectionBase
diff --git a/Database/APR/CollectionBase/CollectionBase/CollectionDescription.h b/Database/APR/CollectionBase/CollectionBase/CollectionDescription.h
index 6ea5ed57953..47ad8118aa8 100755
--- a/Database/APR/CollectionBase/CollectionBase/CollectionDescription.h
+++ b/Database/APR/CollectionBase/CollectionBase/CollectionDescription.h
@@ -1,5 +1,5 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#ifndef COLLECTIONBASE_COLLECTIONDESCRIPTION_H
@@ -573,13 +573,16 @@ namespace pool {
// check if the column contains tokens
virtual bool isTokenColumn( const std::string& columnName, const std::string& method ) const;
- // this version includes the 'mothod name' in the error message
- virtual pool::CollectionColumn* column( const std::string& columnName, const std::string& methodName ) const;
+ // this version includes the 'method name' in the error message
+ virtual pool::CollectionColumn* column( const std::string& columnName, const std::string& methodName );
+ virtual const pool::CollectionColumn* column( const std::string& columnName, const std::string& methodName ) const;
// returns non-const fragment pointer
- virtual pool::CollectionFragment* collectionFragment( int fragmentId, const std::string& method ) const;
+ virtual pool::CollectionFragment* collectionFragment( int fragmentId, const std::string& method );
+ virtual const pool::CollectionFragment* collectionFragment( int fragmentId, const std::string& method ) const;
// returns non-const fragment pointer
- virtual pool::CollectionFragment* collectionFragment( const std::string& fragmentName, const std::string& method ) const;
+ virtual pool::CollectionFragment* collectionFragment( const std::string& fragmentName, const std::string& method );
+ virtual const pool::CollectionFragment* collectionFragment( const std::string& fragmentName, const std::string& method ) const;
public:
/// print out the description (debugging)
diff --git a/Database/APR/CollectionBase/CollectionBase/CollectionFactory.h b/Database/APR/CollectionBase/CollectionBase/CollectionFactory.h
index 3c77051ffeb..3adbcc2f53d 100755
--- a/Database/APR/CollectionBase/CollectionBase/CollectionFactory.h
+++ b/Database/APR/CollectionBase/CollectionBase/CollectionFactory.h
@@ -1,5 +1,5 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#ifndef COLLECTIONBASE_COLLECTIONFACTORY_H
@@ -8,6 +8,7 @@
#include "CollectionBase/ICollection.h"
#include "CollectionBase/CollectionDescription.h"
#include "FileCatalog/IFileCatalog.h"
+#include "CxxUtils/checker_macros.h"
#include <string>
#include <vector>
@@ -27,7 +28,8 @@ namespace pool {
* collection fragments, the latter of which contain a subset of the metadata of a
* full collection.
*/
- class CollectionFactory
+ class ATLAS_NOT_THREAD_SAFE CollectionFactory
+ // not thread-safe due to constness violations wrt the catalog.
{
public:
/// Retrieves the collection factory singleton.
diff --git a/Database/APR/CollectionBase/CollectionBase/CollectionService.h b/Database/APR/CollectionBase/CollectionBase/CollectionService.h
index bee57353970..4800f1a4d1e 100755
--- a/Database/APR/CollectionBase/CollectionBase/CollectionService.h
+++ b/Database/APR/CollectionBase/CollectionBase/CollectionService.h
@@ -1,11 +1,12 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#ifndef COLLECTIONBASE_COLLECTIONSERVICE_H
#define COLLECTIONBASE_COLLECTIONSERVICE_H
#include "CollectionBase/ICollectionService.h"
+#include "CxxUtils/checker_macros.h"
namespace pool {
@@ -21,7 +22,7 @@ namespace pool {
* of the metadata of an existing collection. Note that a class that inherits from this
* interface must also inherit from the SEAL Service base class.
*/
- class CollectionService : virtual public ICollectionService
+ class ATLAS_NOT_THREAD_SAFE CollectionService : virtual public ICollectionService
{
// DECLARE_SEAL_COMPONENT;
diff --git a/Database/APR/CollectionBase/CollectionBase/ICollectionMetadata.h b/Database/APR/CollectionBase/CollectionBase/ICollectionMetadata.h
index d7825418781..7f5b8667cdb 100644
--- a/Database/APR/CollectionBase/CollectionBase/ICollectionMetadata.h
+++ b/Database/APR/CollectionBase/CollectionBase/ICollectionMetadata.h
@@ -1,11 +1,12 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#ifndef ICollectionMetadata_h
#define ICollectionMetadata_h
#include <string>
+#include <memory>
namespace pool {
@@ -35,25 +36,11 @@ namespace pool {
const std::string& key() const { return m_iterator->key(); }
const char * value() const { return m_iterator->value(); }
- ~const_iterator() { delete m_iterator; }
-
- const_iterator( ICollectionMetadataIterator* iter )
- : m_iterator( iter ) {}
-
- const_iterator& operator=( const const_iterator& rhs ) {
- m_iterator = rhs.releaseIter();
- return *this;
- }
-
- protected:
- ICollectionMetadataIterator* releaseIter() const {
- ICollectionMetadataIterator *iter = m_iterator;
- m_iterator = 0;
- return iter;
- }
-
+ const_iterator( std::unique_ptr<ICollectionMetadataIterator> iter )
+ : m_iterator( std::move(iter) ) {}
- mutable ICollectionMetadataIterator* m_iterator;
+ private:
+ std::unique_ptr<ICollectionMetadataIterator> m_iterator;
};
diff --git a/Database/APR/CollectionBase/CollectionBase/ICollectionService.h b/Database/APR/CollectionBase/CollectionBase/ICollectionService.h
index 4d542886c35..025742afce3 100755
--- a/Database/APR/CollectionBase/CollectionBase/ICollectionService.h
+++ b/Database/APR/CollectionBase/CollectionBase/ICollectionService.h
@@ -1,11 +1,12 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#ifndef COLLECTIONBASE_ICOLLECTIONSERVICE_H
#define COLLECTIONBASE_ICOLLECTIONSERVICE_H
#include "CollectionBase/ICollection.h"
+#include "CxxUtils/checker_macros.h"
#include <string>
#include <vector>
@@ -29,7 +30,7 @@ namespace pool {
* Note that a class that inherits from this interface must also inherit from the SEAL
* Service base class.
*/
- class ICollectionService
+ class ATLAS_NOT_THREAD_SAFE ICollectionService
{
public:
/**
diff --git a/Database/APR/CollectionBase/src/CollectionDescription.cpp b/Database/APR/CollectionBase/src/CollectionDescription.cpp
index fa90e718772..b9e9902a8cf 100755
--- a/Database/APR/CollectionBase/src/CollectionDescription.cpp
+++ b/Database/APR/CollectionBase/src/CollectionDescription.cpp
@@ -1,5 +1,5 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#include "CollectionBase/CollectionDescription.h"
@@ -1450,7 +1450,7 @@ pool::CollectionDescription::numberOfColumns( std::string fragmentName ) const
if( !fragmentName.size() ) {
return m_attributeColumnForColumnName.size() + m_tokenColumnForColumnName.size();
}
- CollectionFragment* fragment = collectionFragment( fragmentName, "numberOfColumns" );
+ const CollectionFragment* fragment = collectionFragment( fragmentName, "numberOfColumns" );
return fragment->attributeColumns().size() + fragment->tokenColumns().size();
}
@@ -1481,7 +1481,7 @@ pool::CollectionDescription::columnPtr( const std::string& name ) const
// internal use protected method (when non-const column is needed). throws exceptions
pool::CollectionColumn *
-pool::CollectionDescription::column( const std::string& name, const std::string& method ) const
+pool::CollectionDescription::column( const std::string& name, const std::string& method )
{
std::map< std::string, pool::CollectionColumn* >::const_iterator iColumn;
iColumn = m_attributeColumnForColumnName.find( name );
@@ -1497,6 +1497,22 @@ pool::CollectionDescription::column( const std::string& name, const std::string&
+const pool::CollectionColumn *
+pool::CollectionDescription::column( const std::string& name, const std::string& method ) const
+{
+ std::map< std::string, pool::CollectionColumn* >::const_iterator iColumn;
+ iColumn = m_attributeColumnForColumnName.find( name );
+ if( iColumn == m_attributeColumnForColumnName.end() ) {
+ iColumn = m_tokenColumnForColumnName.find( name );
+ if( iColumn == m_tokenColumnForColumnName.end() ) {
+ std::string errorMsg = "Column with name `" + name + "' does NOT exist.";
+ throw pool::Exception(errorMsg, "CollectionDescription::" + method, "CollectionBase");
+ }
+ }
+ return iColumn->second;
+}
+
+
int
pool::CollectionDescription::numberOfTokenColumns( std::string fragmentName ) const
{
@@ -1609,7 +1625,7 @@ pool::CollectionDescription::attributeColumn( const std::string& columnName ) co
const pool::ICollectionColumn&
pool::CollectionDescription::attributeColumn( int columnId, int fragmentId ) const
{
- CollectionFragment* fragment = collectionFragment( fragmentId, "attributeColumn" );
+ const CollectionFragment* fragment = collectionFragment( fragmentId, "attributeColumn" );
std::vector< pool::CollectionColumn* > columns = fragment->attributeColumns();
if( columnId >= 0 && columnId < (int) columns.size() ) {
@@ -1630,7 +1646,7 @@ pool::CollectionDescription::attributeColumn( int columnId, int fragmentId ) con
const pool::ICollectionColumn&
pool::CollectionDescription::attributeColumn( int columnId, const std::string& fragmentName ) const
{
- CollectionFragment* fragment = collectionFragment( fragmentName, "attributeColumn" );
+ const CollectionFragment* fragment = collectionFragment( fragmentName, "attributeColumn" );
std::vector< pool::CollectionColumn* > columns = fragment->attributeColumns();
if( columnId >= 0 && columnId < (int)columns.size() ) {
return *( columns[ columnId ] );
@@ -1777,6 +1793,22 @@ pool::CollectionDescription::collectionFragment( const std::string& fragmentName
pool::CollectionFragment *
+pool::CollectionDescription::collectionFragment( const std::string& fragmentName, const std::string& method )
+{
+ std::map< std::string, pool::CollectionFragment* >::const_iterator iFragment =
+ m_fragmentForFragmentName.find( fragmentName );
+
+ if( iFragment == m_fragmentForFragmentName.end() ) {
+ std::string errorMsg = "Collection fragment `" + fragmentName + "' NOT found.";
+ throw pool::Exception( errorMsg,
+ "CollectionDescription::" + method,
+ "CollectionBase" );
+ }
+ return iFragment->second;
+}
+
+
+const pool::CollectionFragment *
pool::CollectionDescription::collectionFragment( const std::string& fragmentName, const std::string& method ) const
{
std::map< std::string, pool::CollectionFragment* >::const_iterator iFragment =
@@ -1800,6 +1832,24 @@ pool::CollectionDescription::collectionFragment( int fragmentId ) const
pool::CollectionFragment *
+pool::CollectionDescription::collectionFragment( int fragmentId, const std::string& method )
+{
+ std::map< int, pool::CollectionFragment* >::const_iterator iFragment = m_fragmentForFragmentId.find( fragmentId );
+
+ if ( iFragment == m_fragmentForFragmentId.end() )
+ {
+ std::string errorMsg = "Not using a collection fragment with ID " + std::to_string(fragmentId);
+ throw pool::Exception( errorMsg,
+ "CollectionDescription::" + method,
+ "CollectionBase" );
+ }
+
+ return iFragment->second;
+}
+
+
+
+const pool::CollectionFragment *
pool::CollectionDescription::collectionFragment( int fragmentId, const std::string& method ) const
{
std::map< int, pool::CollectionFragment* >::const_iterator iFragment = m_fragmentForFragmentId.find( fragmentId );
diff --git a/Database/APR/CollectionBase/src/CollectionFactory.cpp b/Database/APR/CollectionBase/src/CollectionFactory.cpp
index 803958d8dea..29de465160d 100755
--- a/Database/APR/CollectionBase/src/CollectionFactory.cpp
+++ b/Database/APR/CollectionBase/src/CollectionFactory.cpp
@@ -1,5 +1,5 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#include "CollectionBase/CollectionFactory.h"
@@ -24,7 +24,7 @@ using namespace std;
using namespace pool;
pool::CollectionFactory pool::CollectionFactory::s_instance;
-static string thisModule( "POOLCollFactory" );
+static const string thisModule( "POOLCollFactory" );
const std::string pool::CollectionFactory::c_fileType = "PoolCollection";
diff --git a/Database/APR/CollectionBase/src/TokenList.cpp b/Database/APR/CollectionBase/src/TokenList.cpp
index 09385868212..17b8cf9a472 100755
--- a/Database/APR/CollectionBase/src/TokenList.cpp
+++ b/Database/APR/CollectionBase/src/TokenList.cpp
@@ -1,5 +1,5 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#include "CollectionBase/TokenList.h"
@@ -99,14 +99,29 @@ pool::TokenList::extend( const std::string& name )
Token&
pool::TokenList::operator[]( const std::string& name )
{
- return const_cast<Token&>( ((const TokenList&)*this)[name] );
+ std::map< std::string, Token* >::iterator iData = m_tokenMap.find( name );
+
+ if( iData == m_tokenMap.end() ) {
+ std::string errorMsg = "Cannot find Token with name `" + name + "' in Token list.";
+ throw pool::Exception( errorMsg,
+ "TokenList::operator[](name)",
+ "CollectionBase" );
+ }
+ return *( iData->second );
}
Token&
pool::TokenList::operator[]( unsigned int index )
{
- return const_cast<Token&>( ((const TokenList&)*this)[index] );
+ if( index >= m_tokenVector.size() ) {
+ std::ostringstream errorMsg;
+ errorMsg << "Cannot find Token with index `" << index << "' in Token list.";
+ throw pool::Exception( errorMsg.str(),
+ "TokenList::operator[](index)",
+ "CollectionBase" );
+ }
+ return *m_tokenVector[index];
}
diff --git a/Database/APR/CollectionBase/test/Factory_test.cxx b/Database/APR/CollectionBase/test/Factory_test.cxx
index 6d389082da3..8bdaa1bbd61 100644
--- a/Database/APR/CollectionBase/test/Factory_test.cxx
+++ b/Database/APR/CollectionBase/test/Factory_test.cxx
@@ -1,5 +1,5 @@
/*
- Copyright (C) 2002-2017 CERN for the benefit of the ATLAS collaboration
+ Copyright (C) 2002-2020 CERN for the benefit of the ATLAS collaboration
*/
#include <cstdio> // For sprintf on gcc45
@@ -15,6 +15,10 @@
#include "CollectionBase/ICollection.h"
#include "CollectionBase/CollectionDescription.h"
#include "CollectionBase/CollectionFactory.h"
+#include "CxxUtils/checker_macros.h"
+
+
+ATLAS_NO_CHECK_FILE_THREAD_SAFETY;
using namespace std;
--
GitLab