CODEWITHSUNDEEP
c++winapicritical-section
chester89
chester89

Reputation: 8617

Critical section - to be or not to be?

I`m writing a chat using WinSock2 and WinAPI functions. And I have a little trouble.
I store the std::vector of client connections on server. When new client connects, new thread starts and all work with the client is done in this new thread. I do not use classes (I know it is not very good) so this list of connections is just defined as global variable.
It seems to me that it can be a situation when several threads try to access this list simultaneously. Though I have not noticed that there are any problems with that, do I need to do something like this:


template 
class SharedVector {
    std::vector vect;
    CRITICAL_SECTION cs;
    SharedVector(const SharedVector& rhs) {}
public:
    SharedVector();
    explicit SharedVector(const CRITICAL_SECTION& CS);
    void PushBack(const T& value);
    void PopBack();
    unsigned int size();
    T& operator[](int index);
    virtual ~SharedVector();
};

template
SharedVector::SharedVector() {
    InitializeCriticalSection(&cs);
}

template
SharedVector::SharedVector(const CRITICAL_SECTION& r): cs(r) {
    InitializeCriticalSection(&cs);
}

template
void SharedVector::PushBack(const T& value) {
    EnterCriticalSection(&cs);
    vect.push_back(value);
    LeaveCriticalSection(&cs);
}

template
void SharedVector::PopBack() {
    EnterCriticalSection(&cs);
    vect.pop_back();
    LeaveCriticalSection(&cs);
}

So, does my situation require using CRITICAL_SECTION and am I just the lucky guy who did not find a mistake?

Upvotes: 2

Views: 2801

Answers (5)

Integer Poet
Integer Poet

Reputation: 747

The biggest question I have for you is architectural. Does the thread for each connection really need direct access to this array of other connections? Shouldn't they really be enqueueing abstract messages of some sort? If each connection thread is conscious of the implementation details of the universe in which it lives, I expect you will run into trouble somewhere down the line.

But let's assume connection threads really do need direct access to each other...

Global variables aren't inherently evil; schools just teach that because it's easier than providing a nuanced understanding. You do have to know the exact implications of a global variable, and it's a decent rule of thumb to assume that a global is the wrong choice until you discover you have no alternative. One alternative which solves a number of problems is to write a function like this: 

SharedVector & GetSharedVector (void)
{
    static SharedVector sharedVector;
    return sharedVector;
}

This buys you more control over when the class gets instantiated than you would have with a global variable, which can be critical if you have dependencies among global variables which have constructors, especially if those global variables spawn threads. It also gives you an opportunity to intercede when someone wants access to this "global" variable rather than just powerlessly suffer while they poke at it directly.

A number of other thoughts also spring to mind. In no particular order...

  • You doubtless already know the template syntax you use here is weird; I'll assume you just typed this code in without compiling it.
  • You need a do-nothing assignment operator in your private section to prevent accidents (for the same reason you have a do-nothing copy constructor there).
  • The explicit copy constructor is broken in a number of ways. It needs to explicitly copy the vector or you'll end up with an empty vector in your new instance of SharedVector. And it should simply initialize its own critical section rather than copy it (and then initialize it). Really for your case it probably doesn't make sense to have this function at all because network connections don't have reasonable copy semantics.
  • You can make exception safety easier by creating a class EnteredCriticalSection whose constructor calls EnterCriticalSection and whose destructor calls LeaveCriticalSection. (It needs to hold onto a reference to the critical section, of course.) This makes it a ton easier to safely serialize your other member functions in the face of exceptions.

Upvotes: 3

Evgeny Lazin
Evgeny Lazin

Reputation: 9423

This code is not exception safe, vector push_back and pop_back methods can throw exceptions and you have potential deadlock here. This methods:

unsigned int size();
T& operator[](int index);

must be synchronized too, because they access data, that can be modified by another thread. For example, you can access data like this:

value = shared_vector[shared_vector.size() - 1];

and at the same time, another thread can do this: 

shared_vector.PopBack();

Upvotes: 3

chester89
chester89

Reputation: 8617

And by the way, it is not a good way to declare this vector as global variable. Will it be better to made it local one?

Upvotes: 0

erikkallen
erikkallen

Reputation: 34421

Yes, you are just lucky never to experience any problems. This is the problem with synchronization issues and race conditions, that the code will work in 99.9% of all cases, and when disaster strikes you won't know why.

I would take away the constructor taking a CRITICAL_SECTION as a parameter since it is not clear without looking at (presumably non-existing) documentation to realize that the constructor will initialize it.

Upvotes: 7

Nemanja Trifunovic
Nemanja Trifunovic

Reputation: 24559

Yes, if you expose a global vector like this, you definitelly need to lock it on any read/write. And yes, that can hurt your performance badly.

Also, one new thread per request is usually not a very good idea either. Why don't you redesign your application to use IO Completion ports instead?

Upvotes: 1

Related Questions