std::for_each blocking

Started by
6 comments, last by Plerion 13 years, 11 months ago
Hello! Im having the following line of code:

void Channel::SendPacketToPlayers(WorldPacket& packet)
{
	m_playerLock.Acquire();
	std::for_each(m_activePlayers.begin(), m_activePlayers.end(), std::tr1::bind(&Player::SendPacket, std::tr1::placeholders::_1, packet));
	m_playerLock.Release();
}
Thats working good two times and after the second message sent std::for_each does not return. What could be the reason for that behaviour? m_activePlayers does not change between the calls (there is 1 active player in the set during each call. The mutex gets released and acquired correctly, ive checked that using outputs. Greetings Plerion
Advertisement
Presumably, Player::SendPacket blocking, perhaps from a network problem. In any case, this is an easy question to answer: Just pause the debugger while it's blocking, and see where it is.
Mhm, debugging gives strange a strange error, it crashes at:
~Mutex(){	DeleteCriticalSection(&m_critical);}


Thats in the destructor of DataBuffer (Baseclass of WorldPacket). To be honest i doubt that there is an error at this point. I will investigate that further :P

Greetings
Probably one or more of:
m_critical wasn't properly initialized
DataBuffer holds Mutex by a pointer, and doesn't follow the rule of 3 (properly), meaning the Mutex was double-deleted
Mutex doesn't follow the rule of 3 (properly), meaning DeleteCriticalSection is called twice on the same m_critical m_criticals of the same value, which the Win32 API may treat as effectively the same critical section, with the problem that entails.
Are you catching (and swallowing) an exception the second time? If the mutex isn't released and is not recursive, then the Acquire() call will block indefinitely.

Even if this isn't the problem, you should definitely look at using the scoped locking idiom to avoid this kind of problem in future.
Hey MaulingMonkey!

You were right, Mutex::Mutex(const Mutex&) gets called (which i didnt expect). Whats basically the correct initialisation in this case? Do i need to Initialize a new critical section for the mutex which gets constructed?

Greetings
Plerion
Quote:Original post by Plerion
Hey MaulingMonkey!

You were right, Mutex::Mutex(const Mutex&) gets called (which i didnt expect). Whats basically the correct initialisation in this case? Do i need to Initialize a new critical section for the mutex which gets constructed?


Well, the whole point of a mutex is that it's shared. So the best solution would be to make sure it isn't copied in the first place. If one thread locks the original mutex and another thread locks the copy, this isn't going to do you any good at all.

If you declare a private copy constructor and assignment operator then the code will fail to compile if you ever attempt to make a copy (or you could use boost::noncopyable, which does this for you).

Another possible solution would be to allow copying of your Mutex type, but have the CRITICAL_SECTION inside a reference counted smart pointer (e.g. boost::shared_ptr). However, I think this is an inferior approach as IMO, it's really important to be able to clearly reason about the lifetime of any synchronization primitives you might have (you don't want your Mutex to be destroyed while someone has a lock on it, for example). Reference counting will probably cloud the issue, further.
Yes, declaring the copy-constructor private was a good idea. As DataBuffer didnt had a private copy-constructor it tried to copy the mutex's. But then i have seen that i actually dont need a mutex in the databuffer. There wont be any synchronisation issues in the databuffer, they wont ever be accessed by different threads as they always will be local variables in a function. Problem solved :)

And to be sure i left the copy-constructor of Mutex private to avoid these problems :D

This topic is closed to new replies.

Advertisement