function insert after a given position thread issue

Question

I'm trying to add a new node in a linked list. The problem is that I have an issue of concurrency and I don't know how to resolve it.

The code:

 void insert(Node* before, unsigned value){
      Node* node = new Node;
      node->loadValue = value;
      Node* after = before->next;
      before->mtx.lock();
      before->next = node;
      before->mtx.unlock();
      after->mtx.lock();
      after->prev = node;
      after->mtx.unlock();
      node->prev = before;
      node->next = after;
 }

Any solution?

Where have you locked `after->mtx`? – MRB Jan 24 '17 at 16:42 — MRB, Jan 24 '17 at 16:42

score 1 · Answer 1 · answered Jan 24 '17 at 17:57

Assuming this isn't homework or some kind of academical/theoretical exercise, I have two easy suggestions to fix your code:

Don't lock/unlock on every single operation. Linked lists are slow enough, you're only worsening the situation with so many lock operations (not to mention you're hardly protecting anything if the list can change between locks). Just lock it once at the beginning and release it when your whole operation (insert, delete, etc) is done. In fact you shouldn't even use something as heavy as mutexes for this, that's why critical sections exist.
You shouldn't be using linked lists anyway, vector based implementations are almost always better in practice. And you certainly shouldn't roll your own, just use the STL provided implementations.

MRB · Answer 2 · 2017-01-24T18:21:13.840

You need to consider many issues when you want to write concurrent code. Concurrent programming need good skills in programming. For your case(linked list) you need to consider:

Exception safe code: When an exception has thrown in your code you need to ensure that all locked mutexs be unlocked.
Avoid dead lock: You must lock mutexs in an order that doesn't put your code in dead lock state.
Avoid race conditions: Race condition occur when two or more thread try to access shared data without any proper synchronization.

Let came back to your example.(I assume that your are using C++11)

Exception safe code: you must always use std::lock_guard or std::unique_lock when you want to use std::mutex:

std::lock_guard<std::mutex> lock(mutex);

std::lock_guard lock a mutex in it's constructor and unlock it in it's destruction. This pattern is RAII technique. So whenever your code throws an exception you are sure that all mutexs will be unlocked automatically.

Dead lock problem: In your specific case you need to choose an order in your class when you lock multiple mutex. If in one function of your class you lock before mutex and then after mutex, in all functions you must follow this order. I mean you can't lock after mutex first and then before mutex. If you don't follow a specific order in your locking pattern you will ended with dead lock.

Race condition problem: In your code you have locked two mutex separately. Consider this example:

One thread enter into insert function and before executing after->mtx.lock(); give it's time slice to another thread. Until now your list would look like bellow state:

Right at this moment another thread enter insert function to add another node right at the same place. After this thread return from the function state of list will be equal to:

And now first thread continue it's work until it return, so change list as:

Now you have a broken list. This is data race in your code.

proper way to implement insertion is:

void insert(Node* before, unsigned value)
{
    Node* node = new Node;
    node->loadValue = value;

    std::lock_guard<std::mutex> before_guard(before->mtx);

    Node* after = before->next;

    std::lock_guard<std::mutex> after_guard(after->mtx);

    before->next = node;
    after->prev = node;
    node->prev = before;
    node->next = after;
 }

An advise: Try to don't write concurrent containers by yourself. It's a professional domain and need much experience to come up with right solution. There is concurrent library like Intel TBB that you can use

Okay, but what if the insertions can be called concurrently but not for the same position? — chi, Jan 24 '17 at 17:55
@chi That doesn't change very much. You need to consider all possible states. May be we can find another state that will broke list. All functions in your class need to be thread safe. you need to consider other functions and all restrictions to write final version. But what I said was a general(and may Incomplete) rule to write concurrent linked list or other containers. — MRB, Jan 24 '17 at 18:16
@MRB, is there another solution without using a global mutex? — Nelly, Feb 24 '17 at 22:13
@Nelly What I said doesn't mean you can't use other solutions. OP want to implement `fine grained concurrent linked list` and I show a scenario that he is doing wrong. In general concurrent data structures can be implemented in three way: `Global mutex`, `Fine grained mutexes`, `Lock free`. The last one is very hard to write in correct way and I don't know if there is a complete solution to it. [Here](http://stackoverflow.com/questions/19609417/atomic-operations-for-lock-free-doubly-linked-list) I try to write one lock free implementation but... I decide to avoid entering this area. — MRB, Feb 25 '17 at 19:33

function insert after a given position thread issue

2 Answers2