Resource Locking / Mutex

[TaikoDragon]

BgInfo: I am creating a MUD style game that is multi-threaded (under win32). Within it there is a large store of players that is serviced in multiple threads. One handling receiving data, and another to process the data (Game Engine). I currently use a Win32 Critical Section to lock a player when some a thread acts on it. I have a lock/unlock function (below) which will suppress attempts to unlock a Critical Section that is not currently locked... since the game depends exclusively on these player objects having the behavior be undefined by allowing the call to LeaveCriticalSection() is not acceptable in my opinion.

I wish to know if I could improve on this, or is there a better way to assure stable operation without suppressing what should be a bug/error?

Locking code:

class Entity {
	// other data members
	CRITICAL_SECTION memberIO;

	// other class stuff
	void AccessVars(bool release = true);
};

void Entity::AccessVars(bool release){
	if(release && memberIO.RecursionCount > 0){
		LeaveCriticalSection(&memberIO);
		return;
	}
	EnterCriticalSection(&memberIO);
	return;
};

PS: Also I'm sure this would apply to any mutex used in this context or for this purpose, so my question is in general.

[helios]

Not only does your function have a race condition, which I'll explain shortly, it's not even correct.

Suppose two threads entered AccessVars() at the same time, and checked RecursionCount at the same time, but then the OS decides to delay execution of one thread, so the other goes ahead and LeaveCriticalSection()s before the other one. If RecursionCount happened to be 1 before the call, then one thread will be about to call LeaveCriticalSection() when RecursionCount is zero.

Now, think what would happen if the caller wanted to unlock the mutex when it shouldn't be unlocked. Because both conditions and ANDed, if any of them fails, the control flow just keeps going. If a thread tries to unlock a mutex when RecursionCount==0, it instead ends up locking it. This will most likely result in a deadlock.

[TaikoDragon]

I guess a solution to the problem you posed is to check that the current thread is the owner of the mutex before trying to unlock it since it can't unlock a mutex that it doesn't own..
Also I do see your point about the case where it fails. I should therefore use a nested if instead of ANDing the conditions together so that in every case where release is true it will never attempt to lock the mutex.

Suppose two threads entered AccessVars() at the same time, and checked RecursionCount at the same time, but then the OS decides to delay execution of one thread, so the other goes ahead and LeaveCriticalSection()s before the other one.

the point of the function is to act as a wrapper for the API calls you would be doing.
and assuming both threads wished to access the player at the same time one of the thread would already have ownership causing the other thread to be in a blocking state waiting for ownership, that is how this particular mutex API is documented as working, this function uses that premise.

[helios]

If you're going to make that assumption, then I don't see the point of making the check at all.

[TaikoDragon]

I was simply stating how Critical Section is supposed to work..
I agree that it there is a race condition, and I wonder how you could work around that since you're dealing with the a mutex to lock the object.

Also I'm wondering what would be considered best practices in this case. Since I know it works.. the game does function, but I rather have clear clean mostly portable code if that is possible.

[helios]

Well, there's two things you can do (there's more, obviously, but these are two best I can think of right now):
1. Directly make the API calls.
2. Wrap them in such a way that if a mutex is locked it has to be unlocked, and that a mutex can't be unlocked without being locked first.

#1 is pretty obvious, so I won't describe it. Here's how you could implement #2:

class MutexWrapper{
	CRITICAL_SECTION *cs;
	MutexWrapper(CRITICAL_SECTION &a):cs(&a){
		EnterCriticalSection(this->cs);
	}
	~MutexWrapper(){
		LeaveCriticalSection(this->cs);
	}
};

T some_thread(CRITICAL_SECTION *cs){
	//..
	{
		MutexWrapper mw(*cs);
		//locked code here
	}
	//...
}

You could even go and generalize the class:

template <typename T,void lock_F(T*),void unlock_F(T*)>
class MutexWrapper{
	T *cs;
	MutexWrapper(T &a):cs(&a){
		lock_F(this->cs);
	}
	~MutexWrapper(){
		unlock_F(this->cs);
	}
};

//Note: Had to do it this way because the functions have a slightly different signature.
//Mostly WinAPI clutter.
void EnterCriticalSection_(CRITICAL_SECTION *cs){
	EnterCriticalSection(cs);
}

void LeaveCriticalSection_(CRITICAL_SECTION *cs){
	LeaveCriticalSection(cs);
}

typedef MutexWrapper<CRITICAL_SECTION,EnterCriticalSection_,LeaveCriticalSection_> CS_MW;

[TaikoDragon]

I see what you're saying and indeed that would be best. Though it wouldn't work my case, the CS is a private member of the Entity class and the class you suggest would go out of scope when AccessVars() returned to the calling function thus unlocking the CS.

Another solution is to completely encapsulate the data container so to access information a function call must be made, but that seems impractical and a waste of CPU time IMHO.

The players carry around their own personal CS objects if that helps any.

I guess this is coming down to the caller being responsible about the lock/unlock process.

[helios]

Wait. Are you saying the mutex is locked and unlocked in different functions? That's just a recipe for disaster.
I guess you could add a transferOwnership() function that took a reference to a MutexWrapper and set a flag in the object telling it not to unlock the mutex when it gets destroyed.

[TaikoDragon]

Not at all. I was talking about using member functions to provide exclusive access to the data in Entity, instead of allowing direct access.
This is a example of the what the proper usable of AccessVars() would be..

void SomeFunction(Entity* e){
	e->AccessVars() // lock the Entity before using it's data
	// do stuff
	e->AccessVars(true); // unlock
}

The release argument of AccessVars seems counter-intuitive now but that is how it currently is..
Anyways, your MutexWrapper preforms the same basic function as the function I'm attempting to critique. I wish to move to boost::mutex objects but they have undefined behavior on unlocking a non-locked object and I don't think you can access data as to if it's currently locked and who owns it.

[helios]

Oh, now I understand what you mean. No, if you want to use my method you'll have to get rid of Entity.