2011-01-11 5 views
1

У меня есть неупорядоченная карта, в которой хранится указатель на объекты. Я не уверен, что я делаю правильную вещь для поддержания безопасности потока.Многопоточность с контейнером STL

typedef std::unordered_map<string, classA*>MAP1; 
MAP1 map1; 
pthread_mutex_lock(&mutexA) 
    if(map1.find(id) != map1.end()) 
    { 
     pthread_mutex_unlock(&mutexA); //already exist, not adding items 
    } 
    else 
    { 
     classA* obj1 = new classA; 
     map1[id] = obj1; 
     obj1->obtainMutex(); //Should I create a mutex for each object so that I could obtain mutex when I am going to update fields for obj1? 
     pthread_mutex_unlock(&mutexA); //release mutex for unordered_map so that other threads could access other object 
     obj1->field1 = 1; 
     performOperation(obj1); //takes some time 
     obj1->releaseMutex(); //release mutex after updating obj1 
    } 
+2

Я не вижу областей, где может возникнуть взаимоблокировка, если только каким-то образом другой поток не может попасть в obj1 до создания '-> getMutex()'. Другими словами, до тех пор, пока вы блокируете 'mutexA' перед доступом к' map1' для чтения или записи, вы должны быть в порядке. Тем не менее, я думаю, что я переместил бы 'pthread_mutex_unlock (& ​​mutexA)' на одну строку, пока 'id' для карты не изменяется в зависимости от содержимого объектов в ней. – milkypostman

+0

Доступны ли объекты, хранящиеся на вашей карте, через несколько потоков для обновлений в поле 1? Разве не лучше сделать объекты потокобезопасными сами по себе, а не загромождать --- извините за это слово --- ваш код с 'obj1-> getMutex'? – Jaywalker

+0

, когда я работал с MSVC, несколько лет назад у нас была многопоточная версия стандартных библиотек. Если вы можете получить потокобезопасную версию stl, она может по крайней мере избавить вас от мьютекса «mutexA» в контейнере «mutexA» – davka

ответ

2

Несколько мыслей.

Если у вас есть один мьютекс на один сохраненный объект, вы должны попытаться создать этот мьютекс в конструкторе для сохраненного объекта. Другими словами, чтобы поддерживать инкапсуляцию, вы должны избегать того, чтобы внешний код управлял этим мьютезом. Я бы преобразовал «field1» в сеттер «SetField1», который обрабатывает мьютекс внутри себя.

Далее, я согласен с комментарием, что вы могли двигаться pthread_mutex_unlock(&mutexA); произойти до obj1->obtainMutex();

Наконец, я не думаю, что вам нужно obtainMutex вообще. Ваш код выглядит так, как будто только одному потоку будет разрешено создавать объект, и поэтому только один поток будет манипулировать содержимым во время создания объекта. Поэтому, если я рассматриваю только тот маленький код, который вы здесь указали, кажется, что мьютекс-на-объект вообще не нужен.

2

Одна проблема, которую я вижу с кодом, заключается в том, что это приведет к проблемам, особенно когда происходят исключения.

obj1->obtainMutex(); //Should I create a mutex for each object so that I could obtain mutex when I am going to update fields for obj1? 
    pthread_mutex_unlock(&mutexA); //release mutex for unordered_map so that other threads could access other object 
    obj1->field1 = 1; 
    performOperation(obj1); 

Если performOperation выдает исключение, то obj1-> releaseMutex(); никогда не будет вызван, таким образом, оставляя объект заблокированным и потенциально приводящим к взаимоблокировкам в будущем. И даже если вы не используете исключения, вы можете использовать какой-нибудь библиотечный код, который вы используете в performOperation. Или вы можете ошибочно когда-нибудь в будущем вставить возврат и забыть разблокировать все принадлежащие блокировки до и так далее ...

То же самое касается вызовов pthread_mutex_lock и pthread_mutex_unlock.

Я бы рекомендовал использовать RAII для блокировки/разблокировки.

I.e. код может выглядеть так:

typedef std::unordered_map<string, classA*>MAP1; 

    MAP1 map1; 

    Lock mapLock(&mutexA); //automatci variable. The destructor of the Lock class 
    //automatically calls pthread_mutex_unlock in its destructor if it "owns" the 
    //mutex 

    if(map1.find(id) == map1.end()) 
    { 
     classA* obj1 = new classA; 
     map1[id] = obj1; 

     Lock objLock(obj); 
     mapLock.release(); //we explicitly release mapLock here 

     obj1->field1 = 1; 
     performOperation(obj1); //takes some time 
    } 

I.e. для ссылки на некоторую минималистичную поддержку резьбы RAAI см. «Современный дизайн C++: общее программирование и образцы дизайна» Андрея Александреску (see here). Другие ресурсы также существуют (here)

В конце концов я попытаюсь описать еще одну проблему, которую я вижу с кодом. Точнее, проблема, которую я вижу, имея методы getMutex и releaseMutex как методы и вызывающие их явно. Предположим, что поток 1 блокирует карту, создает объект вызова getMutex и разблокирует карту. Другой поток (позволяет называть его Thread 2) получает запланированные блокировки выполнения, карта получает итератор к map1 [id] объекта и вызывает releaseMutex() в pObject (т. Е. Предположим, из-за ошибки, код не пытается сначала вызовите getMutex). Теперь Thread 1 получает запланированный вызов и вызывает в какой-то момент releaseMutex(). Таким образом, объект был заблокирован один раз, но дважды выпущен. То, что я пытаюсь сказать, заключается в том, что это будет тяжелая работа, чтобы убедиться, что вызовы всегда правильно спарены перед исключениями, потенциальными ранними результатами, которые не разблокируются и неверно используют интерфейс блокировки объекта. Также Thread 2 может просто удалить pObject, полученный с карты, и удалить его с карты. Затем поток 1 продолжит работу с уже удаленным объектом.

При разумном использовании RAII код будет проще понять (даже короче, если вы сравните наши версии), а также поможет в решении некоторых проблем, перечисленных выше.

+1

Я думаю, что вы можете переместить 'Lock mapLock (& ​​mutexA);' внутри 'if'? – davka

+0

@davka. Наверное, нет, потому что если доступ к карте (делает поиск). Предполагая, что map1 является объектом, разделяемым между несколькими потоками, и другой поток может попытаться вставить в карту, пока поток находится в map1.find (id), доступ к карте должен быть защищен. Можно создать область, содержащую блокировку и следующее, если это будет гарантировать, что mapLock будет выпущен. Возможно, он может явно освободить его после if. Я лениво предположил, что функция, выполняющая доступ к карте, заканчивается после if ;-) – ds27680

+0

, конечно, вы правы. Я сам предложил что-то подобное в своем ответе :) Ну, я сказал, что немного ржавый в многопоточном ... – davka

0

Мысль комбинируя свои комментарии в ответ:

1) При добавлении записи, и, следовательно, изменение емкости, вы не должны позволить доступ из других потоков чтения, так как контейнер может быть в переходе между юридическими государствами. В дополнение, вы не должны изменять контейнер, когда другие потоки его читают. Это требует использования read-write lock. Псевдо-код что-то вроде:

set read lock 
search container 
if found 
    release read lock 
    operate on the found object 
else 
    set write lock 
    release read lock 
    add entry 
    release write lock 
endif 

(это было некоторое время, так как я сделал многопоточное программирование, так что я может быть ржавым на деталях)

2) Когда я работал над MSVC несколько лет назад мы использовали многопоточную версию (т.е. поточную) версию стандартных библиотек . Это может спасти вам все эти неприятности. Не мешало (пока) проверить, существует ли потокобезопасная std также на gcc/Linux.

Смежные вопросы