Board index » cppbuilder » Re: problems making std::map threadsafe using 'critical sections'

Re: problems making std::map threadsafe using 'critical sections'


2005-05-17 04:44:03 AM
cppbuilder60
"Daniel Stevenson" < XXXX@XXXXX.COM >writes:
Quote
I'm sorry if this is the wrong group as I can't find one specific to
my problem.
.cppbuilder.language.cpp seems to be best choice.
Quote
I'm fairly new with using STL map and therefore am having a few problems
attempting to create a threadsafe map so that multiple threads can acces the
map at the same time.

The code I am using is...

//--------------------------------------------------------------------------
-
class CriticalSection
{
private:
CRITICAL_SECTION m_cs;
public:
CriticalSection()
{
::InitializeCriticalSection(&m_cs);
}
CriticalSection(const CriticalSection& rhs)
{
// ignore rhs
::InitializeCriticalSection(&m_cs);
}
~CriticalSection()
{
::DeleteCriticalSection(&m_cs);
}
CriticalSection& operator=(const CriticalSection& rhs)
{
// ignore rhs & do nothing
return *this;
}
void Enter()
{
::EnterCriticalSection(&m_cs);
}
void Leave()
{
::LeaveCriticalSection(&m_cs);
}
};
First some remarks regarding the design of this class:
1) The copy-constructor and the copy-assignment operator are not
consistent. This is almost invariably a recipe for problems. What does
it mean to copy a CriticalSection object? Does it mean anything at
all?
Could you declare the copy-constructor and the copy-assignment
operator private and not implement them?
2) It's very easy to cause deadlocks with Enter() and Leave() in the
public interface of class CriticalSection. Even if you carefully write
a call to Leave() for every call of Enter(), an exception might occur
between the two.
Consider declaring CriticalSectionLock a friend instead.
Quote
template <class Key, class T>
class TMessageMap
{
private:
std::map<Key, T>m_m;
CriticalSection m_cs;

public:
typedef Key key_type;
typedef T mapped_type;
typedef std::pair<const Key, T>value_type;

const value_type& find(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.find(x);
}

const value_type& insert(const value_type& v)
{
CriticalSectionLock lock(m_cs);
return m_m.insert(v); //<<<<--this line is giving the compilation
error as described below
}

unsigned int erase(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.erase(x);
}
};
Side note: I'm all but certain that adding a CriticalSection at this
level is useful; I rather see it at a higher level of abstraction.
E.g., if a thread first calls find(), then erase(), another thread
could call erase() first in your design.
If your design were the typical situation, the Standard Library
implementors would have added this type of synchronization right away.
Quote
//--------------------------------------------------------------------------
-
typedef struct MessageStruct
{
long MessageType;
TDateTime TimeStamp;
} MessageInfo;
//--------------------------------------------------------------------------
-



I instanciate my map using the following..

TMessageMap<String, MessageStruct>MessageQueue;
I'd prefer
typedef TMessageMap<String, MessageStruct>MessageQueue_type;
MessageQueue_type MessageQueue;
...
Quote
When attempting to insert an item into the map i get an error..

MessageStruct Message;
Message.TimeStamp = Now().CurrentTime();
Message.MessageType = mtOAPRegistrationRequest;

std::pair<String, MessageStruct>MessagePair;
MessagePair.first = MessageID;
MessagePair.second = Message;
... and then simply
MessageQueue_type::value_type MessagePair(MessageID,Message);
Quote
MessageQueue.insert(MessagePair); //<<--- my problem is when attempting
to perform an insert

The error reported is

[C++ Error] dtypes.h(406): E2357 Reference initialized with
'pair<_Rb_tree_iterator<pair<const
AnsiString,MessageStruct>,_Nonconst_traits<pair<const
AnsiString,MessageStruct>>>,bool>', needs lvalue of type 'pair<const
AnsiString,MessageStruct>'


Can any advise how I can successfully define my insert and find fuctions so
that they work using the critical sections?
The problem is that the return type of the map's insert() member
function doesn't match that of TMessageMap::insert().
As the error message says, the map's insert() member returns a
std::pair<std::map<...>::iterator, bool>. The bool part of this pair
informs the caller whether the insert() succeeded (i.e. if the map
didn't already contain an element with the key to be inserted), while
the iterator part indicates the position of the newly inserted element
(if the insertion effectively took place) or the element that prevent
the insert() to effectively happen.
The correct form of TMessageMap::insert() depends on the intended
semantics. Is it ok to (attempt to) insert a key that the map already
contains? If yes, the return type probably has to inform the caller if
the insert() succeeded (just like map::insert() does). If no, an
assert() should probably add.
I don't really see how the return type const value_type & could be
useful for the caller, though. Is the following any useful?
void insert(const value_type& v)
{
CriticalSectionLock lock(m_cs);
std::pair<map_type::iterator,bool>const result(m_m.insert(v));
assert(result.second);
}
where map_type is a typedef for std::map<Key, T>.
 
 

Re:Re: problems making std::map threadsafe using 'critical sections'

Quote

>template <class Key, class T>
>class TMessageMap
>{
>private:
>std::map<Key, T>m_m;
>CriticalSection m_cs;
>
>public:
>typedef Key key_type;
>typedef T mapped_type;
>typedef std::pair<const Key, T>value_type;
>
>};

Side note: I'm all but certain that adding a CriticalSection at this
level is useful; I rather see it at a higher level of abstraction.

E.g., if a thread first calls find(), then erase(), another thread
could call erase() first in your design.

Thanks for that, I hadn't even saw that problem, I probably should rectify
it but I reviewed my approach and realised only one thread is performing
find() and erase(), if this changes I will take your advice.
Quote
I don't really see how the return type const value_type & could be
useful for the caller, though. Is the following any useful?

void insert(const value_type& v)
{
CriticalSectionLock lock(m_cs);
std::pair<map_type::iterator,bool>const result(m_m.insert(v));
assert(result.second);
}

where map_type is a typedef for std::map<Key, T>.
Again thanks, I was wondering about that myself but was going to wait until
I had a better understanding of how maps work before working through this.
I've learnt a little about maps and have been able to get most things all
working, but now I'm struggling with a problem associated with find() and
end().
I'm trying to find a specific key within the MessageMap and therefore have
to compare the result with end() to ensure Its a succesful find or not.
my code is..
---------.h
MessageQueue class....
const value_type& find(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.find(x);
}
const value_type& end()
{
returnn m_m.end();
}
--------.cpp
String MessageID = "";
MessageQueue_type::value_type MessagePair =
OAPMessageQueue.find(MessageID);
MessageQueue_type::value_type EndPair = OAPMessageQueue.end();
My struggle is that I've tried the above approach which gives errors as
map.find() returns a different value to MessageMap.find() and the below
approach
---------.h
MessageQueue class....
const map_type::iterator find(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.find(x);
}
const map_type::iterator end()
{
returnn m_m.end();
}
--------.cpp
String MessageID = "";
MessageQueue_type::map_type::iterator MessagePair =
OAPMessageQueue.find(MessageID);
MessageQueue_type::map_type::iterator EndPair = OAPMessageQueue.end();
but this gives and error that map.end() gives a different return type to
MessageMap.end(). I'm under the assumption that both end() and find()
return a iterator but each option is showing me that they are requiring
different return values which has confused me.
the more i study this the less I'm understanding...
Thanks in advance.
 

Re:Re: problems making std::map threadsafe using 'critical sections'

"Daniel Stevenson" < XXXX@XXXXX.COM >writes:
Quote
I've learnt a little about maps and have been able to get most
things all working, but now I'm struggling with a problem associated
with find() and end().

I'm trying to find a specific key within the MessageMap and
therefore have to compare the result with end() to ensure Its a
succesful find or not.

my code is..
---------.h
MessageQueue class....
const value_type& find(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.find(x);
}
The first question that comes to mind is: what should be returned if
the map doesn't have an element with key x?
And the second: which thread removes the element that the returned
reference refers to until the caller dereferences that reference
(because some thread will, sooner or later)? Since the critical
section is left when find() returns, you don't have any protection
against that disastrous scenario.
More, the function should probably be const ...
Quote
const value_type& end()
{
returnn m_m.end();
}
... as should this one.
But since they don't compile, this is not so important :-)
Quote
--------.cpp
String MessageID = "";
MessageQueue_type::value_type MessagePair =
OAPMessageQueue.find(MessageID);
MessageQueue_type::value_type EndPair = OAPMessageQueue.end();
There is no such thing as an "EndPair".
Quote
My struggle is that I've tried the above approach which gives errors
as map.find() returns a different value to MessageMap.find() and the
below approach


---------.h
MessageQueue class....
const map_type::iterator find(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.find(x);
}

const map_type::iterator end()
{
returnn m_m.end();
}
[This code has a basic syntax error; it looks as if you re-type your
code here instead of copying&pasting. Please *always* copy&paste code
that you have just thrown at the compiler, to make sure that what you
write is what actually happens.]
Prefixing both occurences of "map_type::iterator" with "typename"
should cause them to compile, unless I'm overlooking something.
I have mixed feelings about whether that is good, because of the
unsolved synchronization issues.
Quote
--------.cpp
String MessageID = "";
MessageQueue_type::map_type::iterator MessagePair =
OAPMessageQueue.find(MessageID);
MessageQueue_type::map_type::iterator EndPair = OAPMessageQueue.end();


but this gives and error that map.end() gives a different return
type to MessageMap.end().
Please always copy&paste error messages, never re-type them (let alone
describe them in prose).
Quote
I'm under the assumption that both end() and find() return a
iterator but each option is showing me that they are requiring
different return values which has confused me.
The assumption is correct. The devil is in the details. Please post
the error message, and we'll find them.
Quote
the more i study this the less I'm understanding...
Looks like could resort to philosophy if this programming task doesn't
work out :-)
 

{smallsort}

Re:Re: problems making std::map threadsafe using 'critical sections'

XXXX@XXXXX.COM (Thomas Maeder [TeamB]) writes:
I assume it is you who posted the exact same question to
comp.lang.c++.moderated. I don't think that it is particularly nice of
you to let two audiences explain you the same stuff. E.g. why do James
Kanze and I have to teach you the same basic synchronization
principles?
The least thing would have been to point out that you multi-posted.
 

Re:Re: problems making std::map threadsafe using 'critical sections'

Sorry, I Initially posted to there but got frustrated with the delays and
overlooked my mentioning of the multi-post.
..that's not entirely true, my wife was shouting at me to eat dinner and I
had to rush to get this post out and therefore didn't get a chance read it
before sending.
"Thomas Maeder [TeamB]" < XXXX@XXXXX.COM >wrote in message
Quote
XXXX@XXXXX.COM (Thomas Maeder [TeamB]) writes:

I assume it is you who posted the exact same question to
comp.lang.c++.moderated. I don't think that it is particularly nice of
you to let two audiences explain you the same stuff. E.g. why do James
Kanze and I have to teach you the same basic synchronization
principles?

The least thing would have been to point out that you multi-posted.
 

Re:Re: problems making std::map threadsafe using 'critical sections'

Quote
The first question that comes to mind is: what should be returned if
the map doesn't have an element with key x?
Initially I was thinking that my preferred type would be the MessageStruct
(ie, map_type) if the find was successful and NULL if not successful but I'm
not too sure how I will be using the MessageMap in the future, therefore
returning a pair<>is probably still relevant for me at this stage.
Quote
And the second: which thread removes the element that the returned
reference refers to until the caller dereferences that reference
(because some thread will, sooner or later)? Since the critical
section is left when find() returns, you don't have any protection
against that disastrous scenario.
I wasn't hoping to have to think about these issues so early (I know this is
wrong to do so), I was hoping to use this as a learning curve, then when I
was confident I understood what I was doing I would then explore these
things and make the necessary changes...but you've forced me to think about
whether I am going to be creating a disastrous scenario.
...thinking music placed here...
I don't think I am at this stage, I will be having one thread inserting all
the pairs, and another thread finding and deleting. If I was wanting to be
safer, I would have to create a find that returns a copy of the found pair,
and a find&erase function therefore ensuring there is no scenario where one
thread is referencing one element while another is deleting that same
element. Alternatively I could use the CriticalSectionLock at a higher
level for the duration I am referencing the found element (but this is only
good if my processing of the found element is short).
Quote
There is no such thing as an "EndPair".
it was shorter than writing OffTheEndPair or OffTheEndElement
Quote

[This code has a basic syntax error; it looks as if you re-type your
code here instead of copying&pasting. Please *always* copy&paste code
that you have just thrown at the compiler, to make sure that what you
write is what actually happens.]
You caught me, I edited the text, but the results shown below are still the
same.
Quote
Prefixing both occurences of "map_type::iterator" with "typename"
should cause them to compile, unless I'm overlooking something.
...my code snippit from .h [copied and pasted this time] :)
const typename map_type::iterator find(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.find(x);
}
const typename map_type::iterator end() const
{
return m_m.end(); //<<<--error message is displayed for this line
}
...my code snippet from .cpp [copied and pasted]
String MessageID = "";
MessageQueue_type::map_type::iterator MessagePair =
MessageQueue.find(MessageID);
MessageQueue_type::map_type::iterator NullPair = MessageQueue.end();
the resulting error message is ...
[C++ Error] dtypes.h(428): E2034 Cannot convert
'_Rb_tree_iterator<pair<const
AnsiString,MessageStruct>,_Const_traits<pair<const AnsiString,MessageStruct>
Quote
>' to '_Rb_tree_iterator<pair<const
AnsiString,MessageStruct>,_Nonconst_traits<pair<const
AnsiString,MessageStruct>>>'
the same error message is found when I don't use the typename prefix as
well.
Quote
>the more i study this the less I'm understanding...

Looks like could resort to philosophy if this programming task doesn't
work out :-)
How about a philosophical question: Should someone call themselves a
programmer if they don't understand STLs?
 

Re:Re: problems making std::map threadsafe using 'critical sections'

"Daniel Stevenson" < XXXX@XXXXX.COM >writes:
Quote
...my code snippit from .h [copied and pasted this time] :)
:-)
Quote
const typename map_type::iterator find(const key_type &x)
{
CriticalSectionLock lock(m_cs);
return m_m.find(x);
}

const typename map_type::iterator end() const
{
return m_m.end(); //<<<--error message is displayed for this line
}

...my code snippet from .cpp [copied and pasted]

String MessageID = "";
MessageQueue_type::map_type::iterator MessagePair =
MessageQueue.find(MessageID);
MessageQueue_type::map_type::iterator NullPair = MessageQueue.end();


the resulting error message is ...

[C++ Error] dtypes.h(428): E2034 Cannot convert
'_Rb_tree_iterator<pair<const
AnsiString,MessageStruct>,_Const_traits<pair<const AnsiString,MessageStruct>
>>' to '_Rb_tree_iterator<pair<const
AnsiString,MessageStruct>,_Nonconst_traits<pair<const
AnsiString,MessageStruct>>>'
In English, this means: "can't convert from const_iterator to iterator".
The reason is that end() is a const member function. In the scope of
end(), m_m is thus a const map, which means that its end() member
function returns a const_iterator. What do you get if you make end() a
non-const member?
Quote
How about a philosophical question: Should someone call themselves a
programmer if they don't understand STLs?
Programmers are people who program. Good programmers also try to
improve their programming, e.g. by learning new things.
 

Re:Re: problems making std::map threadsafe using 'critical sections'

Quote
In English, this means: "can't convert from const_iterator to iterator".

The reason is that end() is a const member function. In the scope of
end(), m_m is thus a const map, which means that its end() member
function returns a const_iterator. What do you get if you make end() a
non-const member?

That was all it required. Thanks, this is starting to make a lot more sense
now. And after your enterpretation there, I realised that most of my
previous problems have been simple const and non_const pointer conversion
errors.
I don't know where I read this, but is there any problems returning a
non_const pointer in this sort of situation (maybe I heard this is reference
to threads), but I can vaguely remember someone writing that we must always
return const pointers for some reason? Does this statement have any merit?
 

Re:Re: problems making std::map threadsafe using 'critical sections'

"Daniel Stevenson" < XXXX@XXXXX.COM >writes:
Quote
I don't know where I read this, but is there any problems returning
a non_const pointer in this sort of situation (maybe I heard this is
reference to threads), but I can vaguely remember someone writing
that we must always return const pointers for some reason? Does
this statement have any merit?
I don't think so. The decision has to be taken for each case
separately. And when thread synchronization comes into play, it
doesn't matter whether the pointer is to const or to non-const. If
another thread may destruct the object referred to, dereferencing the
pointer will give "interesting" results.