CODEWITHSUNDEEP
c++oopboost-mutex
Sebastian Zander
Sebastian Zander

Reputation: 347

Boost::Mutex in class not thread-safe

I'm learning concurrent programming and what I want to do is have a class where each object it responsible for running its own Boost:Thread. I'm a little over my head with this code because it uses A LOT of functionality that I'm not that comfortable with (dynamically allocated memory, function pointers, concurrency etc etc). It's like every line of code I had to check some references to get it right.

(Yes, all allocated memory is accounted for in the real code!)

I'm having trouble with the mutexes. I declare it static and it seems to get the same value for all the instances (as it should). The code is STILL not thread safe.

The mutex should stop the the threads (right?) from progressing any further in case someone else locked it. Because mutexes are scoped (kind of a neat functionality) and it's within the if statement that should look the other threads out no? Still I get console out puts that clearly suggests it is not thread safe.

Also I'm not sure I'm using the static vaiable right. I tried different ways of refering to it (Seller::ticketSaleMutex) but the only thing that worked was "this->ticketSaleMutex" which seems very shady and it seems to defeat the purpose of it being static.

Seller.h:

class Seller
{
public:     
    //Some vaiables
private:
    //Other variables
    static boost::mutex ticketSaleMutex;      //Mutex definition
};

Seller.cpp:

boost::mutex Seller::ticketSaleMutex;         //Mutex declaration

void Seller::StartTicketSale()
{
    ticketSale = new boost::thread(boost::bind(&Seller::SellTickets, this));

}
void Seller::SellTickets()
{
    while (*totalSoldTickets < totalNumTickets)
    {
        if ([Some time tick])
        {
            boost::mutex::scoped_lock(this->ticketSaleMutex);
            (*totalSoldTickets)++;
            std::cout << "Seller " << ID << " sold ticket " << *totalSoldTickets << std::endl;
        }

    }
}

main.cpp:

int main(int argc, char**argv)
{
    std::vector<Seller*> seller;
    const int numSellers = 10;
    int numTickets = 40;
    int *soldTickets = new int;
    *soldTickets = 0;
    for (int i = 0; i < numSellers; i++)
    {
        seller.push_back(new Seller(i, numTickets, soldTickets));
        seller[i]->StartTicketSale();
    }
}

Upvotes: 3

Views: 1984

Answers (1)

hmjd
hmjd

Reputation: 121961

This will create a temporary that is immediately destroyed:

boost::mutex::scoped_lock(this->ticketSaleMutex);

resulting in no synchronization. You need to declare a variable:

boost::mutex::scoped_lock local_lock(this->ticketSaleMutex);

Upvotes: 12

Related Questions