Non-blocking stack pop

Consider the following code which uses non-blocking semantics to pop a stack:

T Stack<T>::pop( ) 
{ 
    while (1) { 
        if (top == NULL) 
        开发者_运维问答   throw std::string(“Cannot pop from empty stack”);
        Node* result = top;
        if (top && __sync_bool_compare_and_swap(&top, result, top->next)) { 
            return result->data;
        }
    }
}

My concern is that if the thread doing the pop got descheduled just before the 2nd if statement and by the time got back its time slice the stack's empty is my check in 2nd loop good enough to avert a crash? Of course, in the worst case just after the comparison of top with zero the thread could get de-scheduled.

Any opinions appreciated. I am aware of the ABA problem that may also occur.

Firstly, assuming top is volatile and can be changed by another thread at any point, only take its value once per loop so you won't get the rug pulled out from under you:

T Stack<T>::pop( )
{
    while ( 1 ) {
        Node* result = top;
        if ( result == NULL )
            throw std::string ( “Cannot pop from empty stack” );
        // you now know result isn't NULL here 
        if ( __sync_bool_compare_and_swap ( &top, result, result -> next ) ) {
            return result -> data;
        }
    }
}

This still doesn't solve the problem of result having been deleted or otherwise modified between getting top's value and dereferencing it.

You want to use a safe sentinel instead of result -> next, so the logic is:

• If top is null then the queue is empty
• If top is sentinel then something else is pulling off the stack, go do something else useful.
• If top is neither, put sentinel in top, get value from result, put result -> next in top, delete result.

Whether this still counts as wait-free† depends on whether you can find something useful to do in the intermediate state.

There are plenty of papers to read for more efficient ways than using a sentinel - in effect you're simulating a two word CAS with a single CAS, since you need to check something about the state of result as well as the state of top. These are much too complicated to reproduce here.

Not tested in any way:

bool Stack<T>::pop ( T&out )
{
    static const Node* const empty ( 0 );
    static const Node* const sentinel ( empty + 1 );

    while ( true ) {
        Node* result = top;

        if ( result == empty )
            throw std::string ( “Cannot pop from empty stack” );

        if ( result == sentinel )
            // something else is popping, return false and allow 
            // current thread to do some work before retrying
            return false;

        if ( __sync_bool_compare_and_swap ( &top, result, sentinel ) ) {
            // only one thread can CAS from a given result to something, 
            // so we are the only thread which has this value of result
            // hence we can dereference it and delete it/return it to a pool
            //
            // no other thread will change top if top == sentinel, so this
            // CAS can't fail
            if ( !__sync_bool_compare_and_swap ( &top, sentinel, result -> next ))
                throw std::string ( "nobody's perfect" );

            out = result -> data;
            delete result;
            return true;
        }
    }
}

As you only inspect or mutate the pointee of result in the one thread at a time, it should be safe (I haven't used this exact pattern before, and normally I end up thinking of weird cases a couple of days after I design something). Whether this ends up being any better than wrapping a std::deque with pthread_mutex_trylock is worth measuring.

Of course, neither this nor the original is non-blocking anyway - if one thread keeps pulling off the stack, any other thread will go round the loop indefinitely waiting for the CAS to succeed. Fairly unlikely, and easily removed by returning false if the CAS does fail, but you do have to work out what you want the thread to do if it shouldn't wait. If spinning until something can be dequeued is OK, you don't need the return code.

_{† I mostly work on x86/x64, where there's no such thing as lock free code, as CMPXCHG implicitly locks the bus and takes time proportional to the number of caches to sync. So you can have code which doesn't spin and wait, but you can't have code which doesn't lock.}

How about changing things around a bit so that you grab whatever's on the top of the stack, and if it turns out that the top of the stack is empty, then raise the exception.

Right concern.

Moreover, you forget about possibility of memory reordering and caching. For instance, your thread may still see the old value for top, even after other thread has set to NULL.

The topic of lock-free data structures was a vogue topic on Dr. Dobb's while it was still published in paper form. The articles can still be found here.

I don't see how this can possibly work. A problem I see is that when you dereference top in top->next, that memory location might not be valid anymore. Some other thread may have popped the stack and deleted or otherwise modified the item.