Cargo cult programming for thread safe variable access?
-
I found the following class while debugging an application showing infrequent deadlocks:
template <class T>
class Protector
{
private:
CRITICAL_SECTION CS;
T data;public:
Protector() { InitializeCriticalSection(&CS); };
~Protector() { DeleteCriticalSection(&CS); };void operator=(const T& value)
{
EnterCriticalSection(&CS);
try { data = value; } catch(...){}
LeaveCriticalSection(&CS);
};T& __fastcall operator *()
{
EnterCriticalSection(&CS);
T &value = data;
LeaveCriticalSection(&CS);
return value;
}T* __fastcall operator ->()
{
return &data;
}__fastcall operator T()
{
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;
}
};The class is used to wrap member variables supposedly for adding thread safety to them. For instance:
class foo
{
Protector<double> m_foobar;
}It's something like an interlocked exchange based on RAII except for it doesnt make much sense. For me this appears to be cargo cult programming that does not bring any thread safety at all. My main concern are these lines:
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;This appears to be pure nonsense since the lock is acquired just in order to assign the a reference to data. (what on earth could change the address of data while in here?) The lock is then immediately unlocked and the reference returned. What is this supposed to protect? The reference can never change since &data can not change and the value in data is not protected since the lock is released immediately. I think i'm going to get rid of this since it does'nt appear to serve any real purpose. Any other thoughts on that?
-
I found the following class while debugging an application showing infrequent deadlocks:
template <class T>
class Protector
{
private:
CRITICAL_SECTION CS;
T data;public:
Protector() { InitializeCriticalSection(&CS); };
~Protector() { DeleteCriticalSection(&CS); };void operator=(const T& value)
{
EnterCriticalSection(&CS);
try { data = value; } catch(...){}
LeaveCriticalSection(&CS);
};T& __fastcall operator *()
{
EnterCriticalSection(&CS);
T &value = data;
LeaveCriticalSection(&CS);
return value;
}T* __fastcall operator ->()
{
return &data;
}__fastcall operator T()
{
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;
}
};The class is used to wrap member variables supposedly for adding thread safety to them. For instance:
class foo
{
Protector<double> m_foobar;
}It's something like an interlocked exchange based on RAII except for it doesnt make much sense. For me this appears to be cargo cult programming that does not bring any thread safety at all. My main concern are these lines:
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;This appears to be pure nonsense since the lock is acquired just in order to assign the a reference to data. (what on earth could change the address of data while in here?) The lock is then immediately unlocked and the reference returned. What is this supposed to protect? The reference can never change since &data can not change and the value in data is not protected since the lock is released immediately. I think i'm going to get rid of this since it does'nt appear to serve any real purpose. Any other thoughts on that?
I think you're right, the Protector isn't protecting much, however can you relate the occasional deadlocks you're looking for to any of this code? :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
-
I found the following class while debugging an application showing infrequent deadlocks:
template <class T>
class Protector
{
private:
CRITICAL_SECTION CS;
T data;public:
Protector() { InitializeCriticalSection(&CS); };
~Protector() { DeleteCriticalSection(&CS); };void operator=(const T& value)
{
EnterCriticalSection(&CS);
try { data = value; } catch(...){}
LeaveCriticalSection(&CS);
};T& __fastcall operator *()
{
EnterCriticalSection(&CS);
T &value = data;
LeaveCriticalSection(&CS);
return value;
}T* __fastcall operator ->()
{
return &data;
}__fastcall operator T()
{
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;
}
};The class is used to wrap member variables supposedly for adding thread safety to them. For instance:
class foo
{
Protector<double> m_foobar;
}It's something like an interlocked exchange based on RAII except for it doesnt make much sense. For me this appears to be cargo cult programming that does not bring any thread safety at all. My main concern are these lines:
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;This appears to be pure nonsense since the lock is acquired just in order to assign the a reference to data. (what on earth could change the address of data while in here?) The lock is then immediately unlocked and the reference returned. What is this supposed to protect? The reference can never change since &data can not change and the value in data is not protected since the lock is released immediately. I think i'm going to get rid of this since it does'nt appear to serve any real purpose. Any other thoughts on that?
I'm not sure if it's cargo cult programming (based on no understanding, not even believed to be understood by the programmer), but it certainly seems you're right. The very same error is often seen in .NET with properties supposedly providing "thread-safe" access to objects, e.g. public Person Owner { get { lock (this) { return _owner; } } } The lock of course adds only overhead, no thread safety.
-
I think you're right, the Protector isn't protecting much, however can you relate the occasional deadlocks you're looking for to any of this code? :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
I suspect it is a deadlock based on the error report: "The program is freezing once a week". I have no hard evidence for this class beeing the cause of the problem except it's the only place in the program actually locking anything. I don't have more than that so i'm just looking for code that could potentially trigger a deadlock. (i. e. Unnecessary locks beeing acquired) If i'm lucky this was indeed the problem, if not there is at least fewer overhead if i remove the useless locks.
-
I suspect it is a deadlock based on the error report: "The program is freezing once a week". I have no hard evidence for this class beeing the cause of the problem except it's the only place in the program actually locking anything. I don't have more than that so i'm just looking for code that could potentially trigger a deadlock. (i. e. Unnecessary locks beeing acquired) If i'm lucky this was indeed the problem, if not there is at least fewer overhead if i remove the useless locks.
I think the problem is elsewhere. Changing this code will have some influence on the program behavior, if anything it most likely will reduce the frequency of deadlocks, making it even harder to pinpoint the real problem. I would not modify the code, except for adding observability (say logging to a file) until the deadlock is better diagnosed, then fix it and clean up whatever deserves cleaning up, but no sooner for those parts that may have overall influence. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
-
I'm not sure if it's cargo cult programming (based on no understanding, not even believed to be understood by the programmer), but it certainly seems you're right. The very same error is often seen in .NET with properties supposedly providing "thread-safe" access to objects, e.g. public Person Owner { get { lock (this) { return _owner; } } } The lock of course adds only overhead, no thread safety.
Good to see C# is equally vulnerable. I thought of cargo cult programming as the ritual inclusion of code or program structures that serve no real purpose. Locking an algorithm against changes of it's variables is perfectly valid but in this app no external algorithm ever tried to acquire the critical section. (The original code contains public accessors for the lock, i removed them to check if they are used at all. Turned out they were unused...)
-
I think the problem is elsewhere. Changing this code will have some influence on the program behavior, if anything it most likely will reduce the frequency of deadlocks, making it even harder to pinpoint the real problem. I would not modify the code, except for adding observability (say logging to a file) until the deadlock is better diagnosed, then fix it and clean up whatever deserves cleaning up, but no sooner for those parts that may have overall influence. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
If you mean that i should'nt use Shotgun debugging to counter Cargo cult programming you are probably right. (Who is inventing all those terms?) But removing unnecessary code this is something i would consider refacturing and i don't to intend to remove the protector class entirely, just the unnecessary locks.
-
If you mean that i should'nt use Shotgun debugging to counter Cargo cult programming you are probably right. (Who is inventing all those terms?) But removing unnecessary code this is something i would consider refacturing and i don't to intend to remove the protector class entirely, just the unnecessary locks.
iberg wrote:
Who is inventing all those terms?
I'm not.
iberg wrote:
just the unnecessary locks
yeah, they may heavily influence code execution times, and that is why I would not touch them unless I have a work hypothesis that links the problem to them. I'd rather improve observability (add logging) and do whatever it takes to increase the problem frequency (as in reduce unnecessary delays, replace human/external input by automatic/local input, reduce the data set). I rather get 10 crashes/deadlocks a day with lots of logging, than reduce the problem frequency to once a month, which makes it almost impossible to solve at all. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
-
I found the following class while debugging an application showing infrequent deadlocks:
template <class T>
class Protector
{
private:
CRITICAL_SECTION CS;
T data;public:
Protector() { InitializeCriticalSection(&CS); };
~Protector() { DeleteCriticalSection(&CS); };void operator=(const T& value)
{
EnterCriticalSection(&CS);
try { data = value; } catch(...){}
LeaveCriticalSection(&CS);
};T& __fastcall operator *()
{
EnterCriticalSection(&CS);
T &value = data;
LeaveCriticalSection(&CS);
return value;
}T* __fastcall operator ->()
{
return &data;
}__fastcall operator T()
{
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;
}
};The class is used to wrap member variables supposedly for adding thread safety to them. For instance:
class foo
{
Protector<double> m_foobar;
}It's something like an interlocked exchange based on RAII except for it doesnt make much sense. For me this appears to be cargo cult programming that does not bring any thread safety at all. My main concern are these lines:
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;This appears to be pure nonsense since the lock is acquired just in order to assign the a reference to data. (what on earth could change the address of data while in here?) The lock is then immediately unlocked and the reference returned. What is this supposed to protect? The reference can never change since &data can not change and the value in data is not protected since the lock is released immediately. I think i'm going to get rid of this since it does'nt appear to serve any real purpose. Any other thoughts on that?
Maybe the author did have intention of using reference:
T __fastcall operator *()
{
EnterCriticalSection(&CS);
T value = data;
LeaveCriticalSection(&CS);return value;
} -
I found the following class while debugging an application showing infrequent deadlocks:
template <class T>
class Protector
{
private:
CRITICAL_SECTION CS;
T data;public:
Protector() { InitializeCriticalSection(&CS); };
~Protector() { DeleteCriticalSection(&CS); };void operator=(const T& value)
{
EnterCriticalSection(&CS);
try { data = value; } catch(...){}
LeaveCriticalSection(&CS);
};T& __fastcall operator *()
{
EnterCriticalSection(&CS);
T &value = data;
LeaveCriticalSection(&CS);
return value;
}T* __fastcall operator ->()
{
return &data;
}__fastcall operator T()
{
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;
}
};The class is used to wrap member variables supposedly for adding thread safety to them. For instance:
class foo
{
Protector<double> m_foobar;
}It's something like an interlocked exchange based on RAII except for it doesnt make much sense. For me this appears to be cargo cult programming that does not bring any thread safety at all. My main concern are these lines:
EnterCriticalSection(&CS);
T& value = data;
LeaveCriticalSection(&CS);
return value;This appears to be pure nonsense since the lock is acquired just in order to assign the a reference to data. (what on earth could change the address of data while in here?) The lock is then immediately unlocked and the reference returned. What is this supposed to protect? The reference can never change since &data can not change and the value in data is not protected since the lock is released immediately. I think i'm going to get rid of this since it does'nt appear to serve any real purpose. Any other thoughts on that?