Critical section - to be or not to be?

Answer 1

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?

Answer 2

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.

Answer 3

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?

Answer 4

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();

Answer 5

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.