Should calls to container.end() be syncronized?

Hi,

I'm not shure: is the blue version is correct/secure as the red one?

Code:

---

        pthread_mutex_lock(&mEntityTypeMapMutex);
        std::map<int, std::string>::const_iterator resultCiter = EntityTypeMap.find(entity_se_id);
        pthread_mutex_unlock(&mEntityTypeMapMutex);
        if(resultCiter == mEntityTypeMap.end())
        {
                pthread_mutex_unlock(&mEntityTypeMapMutex);
        }
        else
        {
                pthread_mutex_unlock(&mEntityTypeMapMutex);
        }

---

Thank you in advance!

it depends on what other threads could do with the map while you are inside the if

If during your work with resultCiter iterator some other thread could access the map and perform some action that will invalidate the iterator - it is obviously very unsafe to unlock the mutex before you finish your work with the iterator

That is true. Let's assume the code inside the if doen't depend on resultCiter.
Is the comparisation

Code:

---

if(resultCiter == mEntityTypeMap.end())

---

itself secure, independend of what other threads might do with the map?

I do not know the correct answer, but you always can write code like this

Code:

---

pthread_mutex_lock(&mEntityTypeMapMutex);
std::map<int, std::string>::const_iterator resultCiter = EntityTypeMap.find(entity_se_id);
boolean bEnd = (resultCiter == mEntityTypeMap.end());
pthread_mutex_unlock(&mEntityTypeMapMutex);
if(bEnd)
{
}
else
{
}

---

I'd say that .end() is probably a single variable for the map, but it may change at any given time - so for example, if another thread adds an element to the map, that may change the end() - it may only change if it's getting added LAST, but it may also change at any time you add something - I don't know how the implementation of map actually works [it's a binary tree, but what does "end()" mean in this concept - the last element in the map, I presume].

So what happens if the end() has actually changed, and the comparison isn't true when it would have been true for the find operation? I suspect bad things happen (but I don't know that - I can only guess that you need to treat this specially, and thus the problem would be what happens if end() changed inbetween).

This in turn makes me think that it's a bad thing

As an alternative solution, how about copying end() to a "endIter", then unlocking and comparing, e.g.

Code:

---

        pthread_mutex_lock(&mEntityTypeMapMutex);
        std::map<int, std::string>::const_iterator resultCiter = EntityTypeMap.find(entity_se_id);
        std::map<int, std::string>::const_iterator endCiter = EntityTypeMap.end();

        pthread_mutex_unlock(&mEntityTypeMapMutex);
        if(resultCiter == endCiter())
...

---

That way you avoid having two unlock's in different paths of the if/else.

Now, the question becomes, of course, what happens if more elements are added to the map (or removed) during this point, such that end() is now different [or the map is different in some other way]. Does it matter?

Let's say this is a check to see if something should be added to the map, what happens if another thread either removes what you just found or adds an element with the same id as you were using in the search - could that happen, and if so, what happens?

[Who said that threads were easy?]

--
Mats

I see. Fortunately my scenario isn't so worst case as Mats describes. So Vart's solution makes my code a bit cleaner.

Thank you both!

Code:

---

        pthread_mutex_unlock(&mEntityTypeMapMutex);
        if (resultCiter == endCiter)

---

I'd add that this is making a dangerous assumption - the underlying implementation of resultCiter and endCiter may reference internal state of EntityTypeMap.

In posix, all multi-threaded access must be synchronized. The only exception to this rule is if multiple threads are only reading shared data that was written prior to the threads starting. If any thread writes data that another thread might read, synchronization must be used.

When it comes to anything in the std namespace, all access should be done under a mutex. There's no telling when the underlying implementation is going to read or write data.

gg

Quote:

---

Originally Posted by matsp

[Who said that threads were easy?]

---

It can be easy if you go for the simple solution. In this case, use a single mutex to completely serialize all access to the map. But this can be less efficient than it potentially could be. Most of the complexity in multithread code comes from people's attempts to make things more efficient.

In this case I would ask why a map is being used and what sort of access behavior is expected.