Why are Boost thread mutexes so slow compared to Pthreads?
Juha Nieminen
use the Pthreads library if Boost is compiled for a system supporting
the latter. However, according to my tests this either is not so, or
even if it is, Boost is doing something else that Pthreads is not.
The problem is that Boost mutexes are *enormously* slower than
Pthreads mutexes. Why is this?
To test the speed of both mutexes I wrote the small C++ program I have
pasted at the end of this post. Basically it has a function which
increments a variable, and the function uses a mutex lock. (The program
is not multithreaded, but that's not the point of this test.) The
preprocessor macro USE_PTHREADS at the beginning of the program can be
used to choose whether it should use a Pthreads mutex or a Boost mutex.
The function then gets called 1000000000 times in a loop.
I compiled the program using gcc 4.3.1 with "g++ -O3 -march=native"
and in my 3.4GHz Pentium4 linux system the running time of the program was:
- Using Pthreads mutex: 15 seconds.
- Using Boost mutex: 1 minute 30 seconds.
In fact, just the act of linking with -lboost_thread-mt, even if using
the Pthreads library and not the Boost library at all, makes the program
a lot slower for some reason (1 minute 18 seconds). In other words, if
you are using the Pthreads library, giving "-lboost_thread-mt" to the
compiler seems to be very detrimental to its speed. I don't really
understand why, given that I'm not using Boost at all in the Pthreads
version of the program.
-------------------------------------------------------------
#define USE_PTHREADS
#ifdef USE_PTHREADS // ---- Use pthreads locking ----
#include <pthread.h>
namespace
{
struct Mutex
{
pthread_mutex_t mutex;
Mutex() { pthread_mutex_init(&mutex, NULL); }
~Mutex() { pthread_mutex_destroy(&mutex); }
};
struct ScopedLock
{
Mutex& mutex;
ScopedLock(Mutex& m): mutex(m)
{ pthread_mutex_lock(&mutex.mutex); }
~ScopedLock() { pthread_mutex_unlock(&mutex.mutex); }
};
}
#else // ---- Use boost threads locking ----
#include <boost/thread.hpp>
namespace
{
typedef boost::mutex Mutex;
typedef boost::mutex::scoped_lock ScopedLock;
}
#endif
// ------ Main test program -----
namespace
{
Mutex mutex;
unsigned counter = 0;
void incrementCounterWithLock()
{
ScopedLock lock(mutex);
++counter;
}
}
int main()
{
for(unsigned i = 0; i < 1000000000; ++i)
incrementCounterWithLock();
}
Anthony Williams
> I had the impression that the Boost Threads library would internally
> use the Pthreads library if Boost is compiled for a system supporting
> the latter. However, according to my tests this either is not so, or
> even if it is, Boost is doing something else that Pthreads is not.
On pthreads platforms such as Linux, Boost Threads uses the pthreads
library for just about everything, including mutexes. It is a really
thin wrapper, as you will see if you look at
boost/thread/pthread/mutex.hpp
> I compiled the program using gcc 4.3.1 with "g++ -O3 -march=native"
> and in my 3.4GHz Pentium4 linux system the running time of the program was:
>
> - Using Pthreads mutex: 15 seconds.
> - Using Boost mutex: 1 minute 30 seconds.
Wow!
> In fact, just the act of linking with -lboost_thread-mt, even if using
> the Pthreads library and not the Boost library at all, makes the program
> a lot slower for some reason (1 minute 18 seconds). In other words, if
> you are using the Pthreads library, giving "-lboost_thread-mt" to the
> compiler seems to be very detrimental to its speed. I don't really
> understand why, given that I'm not using Boost at all in the Pthreads
> version of the program.
I don't understand either, but it would appear that this is the cause
of your delay, not that boost::mutex is so much slower.
What version of boost are you using?
What were your exact command lines for with/without boost?
Anthony
--
Anthony Williams
Author of C++ Concurrency in Action | http://www.manning.com/williams
Custom Software Development | http://www.justsoftwaresolutions.co.uk
Just Software Solutions Ltd, Registered in England, Company Number 5478976.
Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK
David Schwartz
> I compiled the program using gcc 4.3.1 with "g++ -O3 -march=native"
> and in my 3.4GHz Pentium4 linux system the running time of the program was:
>
> - Using Pthreads mutex: 15 seconds.
> - Using Boost mutex: 1 minute 30 seconds.
>
> In fact, just the act of linking with -lboost_thread-mt, even if using
> the Pthreads library and not the Boost library at all, makes the program
> a lot slower for some reason (1 minute 18 seconds). In other words, if
> you are using the Pthreads library, giving "-lboost_thread-mt" to the
> compiler seems to be very detrimental to its speed. I don't really
> understand why, given that I'm not using Boost at all in the Pthreads
> version of the program.
Profile both versions and post any obvious outliers.
DS
Eric Sosman
> I had the impression that the Boost Threads library would internally
> use the Pthreads library if Boost is compiled for a system supporting
> the latter. However, according to my tests this either is not so, or
> even if it is, Boost is doing something else that Pthreads is not.
>
> The problem is that Boost mutexes are *enormously* slower than
> Pthreads mutexes. Why is this?
Just a guess: Are you sure the pThreads case is actually
using mutexes? Some systems provide a library of do-nothing
"stub" functions to be linked with single-threaded programs;
is it possible you're accidentally using a no-op library?
Here's a brute-force way to check: Modify your test program
slightly so that the test runs in a second thread instead of
in the original thread. If you're able to launch the second
thread at all, you're using a "real" threaded library.
--
Eric Sosman
eso...@ieee-dot-org.invalid
Chris Friesen
> The problem is that Boost mutexes are *enormously* slower than
> Pthreads mutexes. Why is this?
>
> To test the speed of both mutexes I wrote the small C++ program I have
> pasted at the end of this post.
> I compiled the program using gcc 4.3.1 with "g++ -O3 -march=native"
You don't mention...did you also build the non-boost version with -pthread?
Chris
Juha Nieminen
> Just a guess: Are you sure the pThreads case is actually
> using mutexes? Some systems provide a library of do-nothing
> "stub" functions to be linked with single-threaded programs;
> is it possible you're accidentally using a no-op library?
It seems that you are right. If I run the test in a separate thread,
the running time jumps to 1 minute 9 seconds.
It seems that, indeed, gcc is using some dummy mutex code when the
Pthreads library is actually not used for creating threads.
Juha Nieminen
> It seems that you are right. If I run the test in a separate thread,
> the running time jumps to 1 minute 9 seconds.
OTOH, there is still a notable difference: With a Pthreads mutex the
running time is 1 min 9 secs, with a Boost mutex it's 1 min 30 secs.
What could explain this difference?
robert...@yahoo.com
That might just be the Boost wrapper. You are, after all, executing
very little code in this loop other than the pthreads mutex and the
optional Boost wrapper, and you'd only need a modest amount of extra
overhead to make up the extra time.
David Schwartz
My bet is the constant construction and destruction of the lock holder
object. IMO, lock holder objects are evil. If a destructor has serious
side-effects, it should never be called by letting an object go out of
scope. It should always be called with an explicit 'delete' operator.
Consider:
void some_function(void)
{ // call with lock X, call without lock Y
// a
// whole
// bunch
// of
// boring
// code
release_lock();
}
With the 'release_lock' call in there explicitly, if I'm adding code
at the end of the function, I know I must make an intelligent choice
whether to place it before or after the call to 'release_lock'.
However, now:
void some_function(void)
{ // call with lock X, call without lock Y
// a
// whole
// bunch
LockHolder foo;
// boring
// code
}
Now, if I'm adding code to the end of the function, it is not at all
obvious that I hold a lock other than X, perhaps even the Y lock the
comment specifically tells me that I'm not holding at the beginning of
the function. I may never realize that I'm adding code while holding
the 'foo' lock, not just the 'X' lock.
DS
Anthony Williams
> On Dec 18, 3:14 pm, Juha Nieminen <nos...@thanks.invalid> wrote:
>> Juha Nieminen wrote:
>> > It seems that you are right. If I run the test in a separate thread,
>> > the running time jumps to 1 minute 9 seconds.
>
>> OTOH, there is still a notable difference: With a Pthreads mutex the
>> running time is 1 min 9 secs, with a Boost mutex it's 1 min 30 secs.
>> What could explain this difference?
>
> My bet is the constant construction and destruction of the lock holder
> object.
It's possible that that's the cause for the difference --- the program
has more work to do, so takes longer. This is one reason it matters
which version of boost the OP is using. In older versions, the lock
functions were out-of-line functions in a .cpp file, compiled into the
library. In the later versions (1.35 onwards), the lock holder objects
and mutex lock functions are entirely inline, so I would expect them
to be optimized out completely, and reduce to bare
pthread_mutex_lock/unlock calls.
That's just a consequence of long functions. If we replace "LockHolder
foo" with "m.lock()" and add another call to "m.unlock()" in the
"boring code", it's not necessarily obvious that the lock on m is
being acquired either. On the other hand, if the function is less than
20 lines or so, you can see whether or not it acquires the lock
because all the code fits on one screenful in your editor.
Lock holder objects are not evil, they are a Good Thing (TM), since
they ensure that the lock is released when the function exits, without
you having to add explicit unlock calls by every return
statement. They also ensure the lock is released if an exception is
thrown. Fewer lines of code => easier to maintain, and fewer bugs.
JC
That's a result of how you wrote your code. You could easily avoid
situations like that with explicit scope and a comment:
void some_function(void)
{ // call with lock X, call without lock Y
// a
// whole
// bunch
{
LockHolder foo;
// boring
// code
} // <-- foo LockHolder is released.
}
That removes, or at least significantly reduces, the risk of
accidentally adding code still in the lock.
Jason
Juha Nieminen
> My bet is the constant construction and destruction of the lock holder
> object. IMO, lock holder objects are evil. If a destructor has serious
> side-effects, it should never be called by letting an object go out of
> scope. It should always be called with an explicit 'delete' operator.
And then your function exits in an unexpected place, the 'delete' is
never called, the lock never released, and your program malfunctions.
And this might never be caught in testing.
Dave Butenhof
Sure; but the point is that, as you've exited "in an unexpected place"
where you held a mutex, you're very likely also leaving shared data in
an "unexpected state". In this case, transparently releasing the mutex
on exit, without restoring proper data state, is just "covering a trail
of deceit and betrayal" that will make it difficult to discover the root
cause of eventual (and possibly serious) consequences.
When this programming error occurs, you'll have a far easier time
isolating and fixing the bug if the failure mode is "deadlock because
the mutex wasn't released" instead of "corrupted data". (Among other
things, a reasonable concurrency debugging package will be able to tell
you where the mutex was last locked, pointing you directly at the bad
routine.)
However, while David definitely has a point here, I totally disagree
that lock guard objects are "evil" -- they're merely a very sharp tool
that, while extremely valuable for the right job, can easily be misused
and is absolutely dangerous in incautious hands. (We can say that of
threads, mutexes, and even C++, so this should be no surprise.)
The problem is that it's too easy to get lazy and depend on the guard
object to solve your problem. And that's simply a dangerous
misunderstanding of the tool.
Eric Sosman
... between 69 seconds (pThreads) and 90 seconds (Boost).
Over a span of 1E9 lock/unlock pairs. 21 nanoseconds per pair,
about 10-11 nsec per operation, about 36 clock cycles on your
3.4GHz CPU.
Assume (without proof) that on a pThreads system the Boost
functions just forward the program's calls to their pThreads
counterparts, maybe with a little rearrangement of the argument
lists and some trivial translations of returned results. Does
36 cycles sound like a reasonable budget for this activity?
How many cycles does it take to call a do-nothing function
and return from it? (No inlining, please: Let's have all the
statutory stack maintenance and calling conventions.)
--
Eric Sosman
eso...@ieee-dot-org.invalid
Anthony Williams
> Assume (without proof) that on a pThreads system the Boost
> functions just forward the program's calls to their pThreads
> counterparts,
No need to assume --- that's what they do.
> maybe with a little rearrangement of the argument
> lists and some trivial translations of returned results. Does
> 36 cycles sound like a reasonable budget for this activity?
It would indicate that the compiler was doing little optimization on
that, but I would have thought it reasonable.
Juha Nieminen
>> And then your function exits in an unexpected place, the 'delete' is
>> never called, the lock never released, and your program malfunctions.
>> And this might never be caught in testing.
>
> Sure; but the point is that, as you've exited "in an unexpected place"
> where you held a mutex, you're very likely also leaving shared data in
> an "unexpected state".
If you have gone through the trouble of making your lock scoped, why
wouldn't you do the same for all the other data used in the function as
well?
I really don't buy this "let's deliberately make unsafe code so that
it will be easier to find out if a function is exited unexpectedly" type
of design. That's only asking for trouble.
I really prefer the "let's design this function so that it will always
work correctly even if I forget putting a 'delete' at some exit point,
and even if something the function calls throws an exception" type of
design. In other words, "this function never leaks resources" design.
If what you want is that the programmer *must* call some resource
freeing function manually (for whatever reason), the correct way of
doing that is to create a scoped object which executes an assertion
failure if the freeing function has not been called when the object goes
out of scope. (OTOH, I really don't see the point.)
> In this case, transparently releasing the mutex
> on exit, without restoring proper data state, is just "covering a trail
> of deceit and betrayal" that will make it difficult to discover the root
> cause of eventual (and possibly serious) consequences.
No, what it does is make your code safer by using a managed resource.
Basically you are telling the compiler "free this resource when it's not
used anymore in this function". It's a bit like some kind of
deterministic garbage collection.
It's not like C++ would be the only language which supports doing
this. For example in C# you have the "using" block for this exact
purpose: To have scoped objects which get finalized when their scope ends.
> When this programming error occurs, you'll have a far easier time
> isolating and fixing the bug if the failure mode is "deadlock because
> the mutex wasn't released" instead of "corrupted data".
If your data gets corrupted because the function was exited
unexpectedly, then your design sucks.
> The problem is that it's too easy to get lazy and depend on the guard
> object to solve your problem. And that's simply a dangerous
> misunderstanding of the tool.
I don't even understand what this has to do with my original question
about the speed efficiency of Boost mutexes vs. Pthreads mutexes.
JC
> Juha Nieminen wrote:
> > David Schwartz wrote:
> >> My bet is the constant construction and destruction of the lock holder
> >> object. IMO, lock holder objects are evil. If a destructor has serious
> >> side-effects, it should never be called by letting an object go out of
> >> scope. It should always be called with an explicit 'delete' operator.
>
> > And then your function exits in an unexpected place, the 'delete' is
> > never called, the lock never released, and your program malfunctions.
> > And this might never be caught in testing.
>
> Sure; but the point is that, as you've exited "in an unexpected place"
> where you held a mutex, you're very likely also leaving shared data in
> an "unexpected state".
Then you've messed up your design. If you use the same kinds of auto-
release idea, and have well-defined invariants that you stick to,
you'll never run into this situation. It's part of the art of writing
exception-safe code.
> In this case, transparently releasing the mutex
> on exit, without restoring proper data state, is just "covering a trail
> of deceit and betrayal" that will make it difficult to discover the root
> cause of eventual (and possibly serious) consequences.
Only if you've blazed a trail of deceit and betrayal to begin with.
There are plenty of simple techniques to make sure everything stays
well defined at any point. Doing it right with proper exception-safety
from the start means you don't *have* to dig to find the root cause of
a problem -- the problem just doesn't exist.
> When this programming error occurs, you'll have a far easier time
> isolating and fixing the bug if the failure mode is "deadlock because
> the mutex wasn't released" instead of "corrupted data". (Among other
> things, a reasonable concurrency debugging package will be able to tell
> you where the mutex was last locked, pointing you directly at the bad
> routine.)
>
> However, while David definitely has a point here, I totally disagree
> that lock guard objects are "evil" -- they're merely a very sharp tool
> that, while extremely valuable for the right job, can easily be misused
> and is absolutely dangerous in incautious hands. (We can say that of
> threads, mutexes, and even C++, so this should be no surprise.)
In fact, it's not the lock guard that's being misused, but rather
failure to maintain exception-safety in other parts of the code that
allow things to be left in an undefined state.
If an exception at an "unexpected" point leaves shared data in a
"corrupt" state, the fact that you did or didn't use lock guards is
irrelevant. Not using them doesn't eliminate the corruption, you still
have a problem, and the problem is not the lock guard, but the fact
that you weren't aware of the state of your application at a possible
failure point.
> The problem is that it's too easy to get lazy and depend on the guard
> object to solve your problem. And that's simply a dangerous
> misunderstanding of the tool.
The guard object doesn't solve your problem, because it's unrelated to
your problem. Your problem is your code wasn't exception-safe, and now
you're left with corrupt data. The guard itself, in fact, worked
perfectly -- it did exactly what it was supposed to do in a completely
well-defined manner: it ensured a resource was released when a
function exited for any reason.
Jason
JC
More accurately, I should say, it ensured a resource was left in a
well-defined state when a function exited for any reason -- and that's
what you should do for *all* resources. If the postconditions of a
function are:
- Shared object A is in some well-defined state.
- Mutex M is released.
And the function throws an exception, leaving shared object A in an
undefined state but with mutex M released, where is the problem? It's
not in the lock guard, that's the part that *worked*.
Sorry about the extra email,
Jason
Ulrich Eckhardt
How about checking returnvalues and converting them to exceptions? Also, you
can actually look at the Boost code or watch it in a debugger to see what
it does.
Uli
David Schwartz
> That's just a consequence of long functions. If we replace "LockHolder
> foo" with "m.lock()" and add another call to "m.unlock()" in the
> "boring code", it's not necessarily obvious that the lock on m is
> being acquired either.
I don't think you understood my example. The issue was not that the
lock on 'm' was acquired somewhere in the middle of the function. It
was that code added at the end of the function will run with the lock
held.
> On the other hand, if the function is less than
> 20 lines or so, you can see whether or not it acquires the lock
> because all the code fits on one screenful in your editor.
But you can't see where the lock is released. All you'll see is '}'
which looks a lot different from 'l.Unlock()'.
> Lock holder objects are not evil, they are a Good Thing (TM), since
> they ensure that the lock is released when the function exits, without
> you having to add explicit unlock calls by every return
> statement. They also ensure the lock is released if an exception is
> thrown. Fewer lines of code => easier to maintain, and fewer bugs.
You can ensure locks are released if an exception is thrown without
evil lock holder objects. You are correct that they mean you don't
have to add explicit unlock calls by every return statement, but I
believe that's a good thing. What if code later needs to be added
after the lock is released but before the 'return'?
If your issue with having to unlock before 'return' is the risk that
some obscure code path will fail to release the lock and cause a bug,
again, that can be solved without evil lock holder objects.
For example, you can create lock holder objects that assert in debug
builds if they are destroyed without releasing the lock. In release
builds, they can simply release the lock. That way, a forgotten
'unlock' does not cause a release build to deadlock. This has none of
the evil of typical auto-unlock lock holders.
DS
David Schwartz
> That's a result of how you wrote your code. You could easily avoid
> situations like that with explicit scope and a comment:
>
> void some_function(void)
> { // call with lock X, call without lock Y
> // a
> // whole
> // bunch
> {
> LockHolder foo;
> // boring
> // code
> } // <-- foo LockHolder is released.
>
> }
>
> That removes, or at least significantly reduces, the risk of
> accidentally adding code still in the lock.
That's just plain silly. How is that any better than the self-
commenting "foo.Unlock()"?
Code that is self-documenting is way preferable to code that requires
comments to understand.
DS
David Schwartz
Quite the reverse, the lock holder object hides this. It is trivial to
create a system that detects this in testing and makes it "just work"
in release. (For example, consider a *proper* lock holder object that
asserts if its destructor is called with the lock held.)
DS
David Schwartz
> If what you want is that the programmer *must* call some resource
> freeing function manually (for whatever reason), the correct way of
> doing that is to create a scoped object which executes an assertion
> failure if the freeing function has not been called when the object goes
> out of scope. (OTOH, I really don't see the point.)
I agree. There are several points:
1) It makes it possible to later add code either before or after the
freeing function is called. When the freeing function reclaims memory,
this almost never matters. When the freeing function releases a lock,
it almost always does.
2) It ensures that looking at the code, you can tell what code runs
holding the lock and what code runs not holding it. Code that can run
with or without a particular lock is very rare. So it's vital to know
when locks are released. The '}' operator gives you no clue that a
lock is being released.
3) If you forget to call a freeing function, you may have forgotten
other things as well. Hiding the forgetting is not a good idea.
(Failing gratuitously is not good either, but hiding omissions from
the programmer in debug builds is not a good idea.)
The point is that holding a lock and not holding a lock are seriously
different execution conditions. Something needs to indicate this
difference, and '}' doesn't cut it.
DS
Barry Kelly
> On Dec 19, 12:09 am, Anthony Williams <anthony....@gmail.com> wrote:
>
> > On the other hand, if the function is less than
> > 20 lines or so, you can see whether or not it acquires the lock
> > because all the code fits on one screenful in your editor.
>
> But you can't see where the lock is released. All you'll see is '}'
> which looks a lot different from 'l.Unlock()'.
This is just a function of the programmer's familiarity with the RAII
idiom. I don't think you can describe it as intrinsically good or evil.
-- Barry
David Schwartz
Well, I did. So I can.
It's intrinsically evil to replace:
l.Unlock();
}
with:
} // implicitly unlocks 'l'
Self-commenting code is always better than code that requires
comments.
DS
Barry Kelly
> Self-commenting code is always better than code that requires
> comments.
That's a ridiculous statement.
What code requires comments? In this context, it appears it's the code
that uses abstractions you don't know about.
What code is "self-commenting"? The code that uses abstractions you do
understand.
So, should all code be written such that no new abstractions are
introduced? No new functions? No new types? After all, we could make do
with an array and a little pointer manipulation, and then put all our
code in main().
Do you comment for loops? Or do you only use while loops, since their
semantics don't require as much explanation? Or do you go even further,
and only use goto?
Do you avoid ++ and --, given that they have simpler, more easily
explained alternative versions?
Do you completely eschew exceptions, and thus never write a function
that returns anything other than an error code? (Since a major reason to
use RAII is to correctly clean up when exiting a scope wherever
possible.)
Abstractions are essential to writing large programs. It's well
established that bug incidence is directly correlated to the textual
size of the source. Thus it's imperative that one be able to encapsulate
reusable code and idioms and reference them symbolically, to increase
code density. The other side of the same coin is that the reader of the
code must learn the abstractions being used before they can fully
understand the code, but that's not unreasonable; it applies just as
much to the base language, as it does to the RTL, and to the other
libraries linked in, etc.
Torsten Robitzki
David Schwartz wrote:
> On Dec 19, 12:09 am, Anthony Williams <anthony....@gmail.com> wrote:
> I don't think you understood my example. The issue was not that the
> lock on 'm' was acquired somewhere in the middle of the function. It
> was that code added at the end of the function will run with the lock
> held.
This realy sounds to me like functions that are to complex or like
people modifying code they do not understand. If one needs a m.unlock()
at the end of the function to remind them to be carefully while modifing
multithreaded code, then we have realy other problems.
> But you can't see where the lock is released. All you'll see is '}'
> which looks a lot different from 'l.Unlock()'.
Add a comment to the closing } ;-)
> What if code later needs to be added
> after the lock is released but before the 'return'?
The usual approache would be to indent the code, add braces and thus
adding a new scope, where the mutex is locked and add the new code
behind that new scope. If the function gets to complex, refactor it.
> If your issue with having to unlock before 'return' is the risk that
> some obscure code path will fail to release the lock and cause a bug,
> again, that can be solved without evil lock holder objects.
It's not about the obscure code pathes, but the usal ones, from whom the
most can cause errors. Using exceptions to report this kinds of errors
simply leads to scope guards or you will write mutch more code to handle
errors that one have to write with C.
> For example, you can create lock holder objects that assert in debug
> builds if they are destroyed without releasing the lock. In release
> builds, they can simply release the lock. That way, a forgotten
> 'unlock' does not cause a release build to deadlock. This has none of
> the evil of typical auto-unlock lock holders.
Thus ignoring the bug in release code ;-)
best regards
Torsten
--
kostenlose Wirtschaftssimulation: http://www.financial-rumors.de
David Schwartz
> David Schwartz wrote:
> > Self-commenting code is always better than code that requires
> > comments.
> That's a ridiculous statement.
I'm sorry you feel that way.
> What code requires comments? In this context, it appears it's the code
> that uses abstractions you don't know about.
Code that has non-obvious side-effects requires commenting.
> What code is "self-commenting"? The code that uses abstractions you do
> understand.
Code that has no obvious side-effects does not require commenting.
> So, should all code be written such that no new abstractions are
> introduced? No new functions? No new types? After all, we could make do
> with an array and a little pointer manipulation, and then put all our
> code in main().
All code should be written such that there are no non-obvious
significant side-effects. If there are, they should be commented, but
it's preferable that the code be self-commenting.
> Do you comment for loops? Or do you only use while loops, since their
> semantics don't require as much explanation? Or do you go even further,
> and only use goto?
Actually, I generally use whichever structure is clearer.
> Do you avoid ++ and --, given that they have simpler, more easily
> explained alternative versions?
I actually do in cases where they are non-obvious. I've grudgingly
come to accept things like '*ptr++=0;' because I think it's common
enough that it's obvious. But at least none of these things require
you to look to other lines of code to figure out what they mean.
> Do you completely eschew exceptions, and thus never write a function
> that returns anything other than an error code? (Since a major reason to
> use RAII is to correctly clean up when exiting a scope wherever
> possible.)
Actually, I do almost completely eschew exceptions. Part of the reason
for that is that they weren't widely available when my coding style
matured. But I still don't particularly like them. (I don't go
overboard and insist that every function have a single 'return'
statement though. I think that's as absurd as prohibiting exceptions
where they do make code clearer.)
IMO, code clarity is perhaps the very highest coding priority.
> Abstractions are essential to writing large programs. It's well
> established that bug incidence is directly correlated to the textual
> size of the source. Thus it's imperative that one be able to encapsulate
> reusable code and idioms and reference them symbolically, to increase
> code density. The other side of the same coin is that the reader of the
> code must learn the abstractions being used before they can fully
> understand the code, but that's not unreasonable; it applies just as
> much to the base language, as it does to the RTL, and to the other
> libraries linked in, etc.
I agree with all of that. I just don't think it's ever going to be
clear that '}' releases a lock that was acquired 50 lines ago without
a comment. And the comment might as well release the lock with code,
since self-commenting code is always better than code that requires
comments, all other things being equal. In this case, all things are
not equal, there's an additional factor favoring the explicit unlock
-- it's easier to add code either before or after it occurs.
DS
Torsten Robitzki
But if you are using C++ to write your software, you are used to read
software like written above.
If you see a mutex being locked, without a scope guard responsible to
unlock that mutex, one have to check every path of execution. In the
best case, there will be only on return in the function and one have to
make sure, that close to the lock() call a try blocks begins and that
every function call up to that try have the guaranty to not throw any
exception. In the catch clause, the mutex will be unlocked and hopefully
the author did not forgot to rethrow the exception. Now add a second
mutex and things will get really complicated.
One can love c++ exception (like I do) or hate them, but one can't
ignore them. So one have to take into account that nearly every function
can throw an exception and have to code according to it.
Juha Nieminen
> 1) It makes it possible to later add code either before or after the
> freeing function is called. When the freeing function reclaims memory,
> this almost never matters. When the freeing function releases a lock,
> it almost always does.
Or you could simply enclose the scoped lock into a {} block.
In fact, that will probably make the design of your function better
because you won't have strange interleaved scopes (eg. objects which
were created after the lock outliving it).
Also you don't have to worry about surprising exit points (eg. caused
by thrown exceptions).
> 2) It ensures that looking at the code, you can tell what code runs
> holding the lock and what code runs not holding it.
How does it ensure it? Consider:
if(someFunction())
lock.release();
someOtherCodeHere(); // Locked or not?
Yeah, "don't write code like that". However, it proved the point that
using manual unlocking does not ensure anything.
Note that with a scoped lock you can still unlock manually if you want
(at least if the lock object has a public unlocking member function).
It's just that the scoped lock makes sure the lock is released when the
function is exited, no matter how it's exited.
> Code that can run
> with or without a particular lock is very rare. So it's vital to know
> when locks are released. The '}' operator gives you no clue that a
> lock is being released.
So better to risk leaking the lock.
> 3) If you forget to call a freeing function, you may have forgotten
> other things as well.
No, it means that your program design is flawed.
I really don't understand all this "let's deliberately make unsafe
code so that it will give us a hint that we *might* have a leak
somewhere" design.
Personally I prefer "let's deliberately make the function not leak
anything with the help of the compiler" design.
> Hiding the forgetting is not a good idea.
It's not hiding anything. You don't "hide" the fact that if you create
an integer at the beginning of the function, it's being freed when the
function exits. You just *know* it.
The exact same thing happens with the scoped lock: When you create a
scoped lock at the beginning of the function, you *know* it will be
released when the function exits. There's no doubt nor mistake possible.
> The point is that holding a lock and not holding a lock are seriously
> different execution conditions. Something needs to indicate this
> difference, and '}' doesn't cut it.
Ridiculous.
Juha Nieminen
>> And then your function exits in an unexpected place, the 'delete' is
>> never called, the lock never released, and your program malfunctions.
>> And this might never be caught in testing.
>
> Quite the reverse, the lock holder object hides this.
Hides what?
> It is trivial to
> create a system that detects this in testing and makes it "just work"
> in release.
And it's trivial to create a scoped lock which never leaks. I really
prefer writing code which I *know* works even without testing, rather
than relying on testing to tell me if I made a mistake.
> (For example, consider a *proper* lock holder object that
> asserts if its destructor is called with the lock held.)
If I was forced at gunpoint to use such a lock, I would write a
wrapper class for it, which automatically releases the lock when the
function is exited. Problem solved, no leaks possible.
I really don't buy this whole "let's deliberately write unsafe code
because, you know, if it fails it might indicate that our program sucks".
Ian Collins
> Dave Butenhof wrote:
>
>> When this programming error occurs, you'll have a far easier time
>> isolating and fixing the bug if the failure mode is "deadlock because
>> the mutex wasn't released" instead of "corrupted data".
>
> If your data gets corrupted because the function was exited
> unexpectedly, then your design sucks.
>
is held? Wow, I'm impressed.
You are effectively saying your function will never exit unexpectedly,
so why bother with a scoped lock at all?
I'm with Dave and David, I used scoped locks many years ago when C++ was
a new toy and soon learned the error of my ways.
--
Ian Collins
JC
> On Dec 19, 1:04 am, JC <jason.cipri...@gmail.com> wrote:
>
>
>
> > That's a result of how you wrote your code. You could easily avoid
> > situations like that with explicit scope and a comment:
>
> > void some_function(void)
> > { // call with lock X, call without lock Y
> > // a
> > // whole
> > // bunch
> > {
> > LockHolder foo;
> > // boring
> > // code
> > } // <-- foo LockHolder is released.
>
> > }
>
> > That removes, or at least significantly reduces, the risk of
> > accidentally adding code still in the lock.
>
> That's just plain silly. How is that any better than the self-
> commenting "foo.Unlock()"?
Because the self-commenting "foo.Unlock()" isn't exception safe.
Using "foo.Lock()" and "foo.Unlock()" gives you self-documenting code,
you are right. The price you pay is no automatic exception / return
safety (you must remember to unlock at every point, very error prone),
no enforcement of matched lock/unlock pairs (a subtle error in your
logic could unlock the same mutex twice), and when adding new code you
*must* scan the existing function in great detail so that you know
what resources are allocated at any given point and can free them
accordingly -- this is especially difficult with a complex function
(where noticing the presence of "foo.Lock()" at a glance is not enough
"self-documentation" to know what's going on) and especially error
prone, as you should be able to see.
Using a lock holder with no explicit scope or comment, on the other
hand, does not give you self-documenting code, you are right again.
You gain exception safety, return safety, a guarantee of a 1:1 lock/
unlock in all situations, and the ability to add/modify code without
any possibility of forgetting to release the resource or releasing a
resource that was never acquired. On the other hand, the price you pay
is since you did not use explicit scoping, and it's not implicitly
obvious what's going on, you run the risk of adding new code with the
resource acquired when the resource should not actually still be
acquired.
However, explicit scope and a comment mitigates that risk. Therefore,
using a lock holder WITH explicit scope and a good comment at the
closing brace gains you all of the above, but the price is you don't
have "self-documenting code". IMHO, the benefits are enormous compared
to the cost of having to type "// Unlocked" instead of ".Unlock()".
The "self-documentation" is arguably no more convenient than a short
comment, anyways.
Really, the *only* compelling reason to not use lock holders is if you
are using scopes that don't fit cleanly inside the locking scope, e.g.
a function like pthread_cond_wait would be hard to implement using a
scope-based lock holder for the mutex instead of just a "raw" mutex.
The way I look at it, preferring to type periods and parentheses
instead of forward slashes is not a compelling reason -- if I write
code that happens to document itself that's a pleasant side-effect of
the way my code was written, but I find that making sacrifices to
*force* code to document itself is generally not worth it.
Jason
Ian Collins
> Really, the *only* compelling reason to not use lock holders is if you
> are using scopes that don't fit cleanly inside the locking scope, e.g.
> a function like pthread_cond_wait would be hard to implement using a
> scope-based lock holder for the mutex instead of just a "raw" mutex.
> The way I look at it, preferring to type periods and parentheses
> instead of forward slashes is not a compelling reason -- if I write
> code that happens to document itself that's a pleasant side-effect of
> the way my code was written, but I find that making sacrifices to
> *force* code to document itself is generally not worth it.
>
up-thread is you can't (at least not without a great deal of effort -
more than they save) guarantee the resource protected by the lock is in
a sane state in the event of an exception.
By all means use lock holders, but use the destructor to assert the lock
has been released not to release the lock.
--
Ian Collins
Ulrich Eckhardt
> It's intrinsically evil to replace:
>
> l.Unlock();
> }
>
> with:
>
> } // implicitly unlocks 'l'
>
> Self-commenting code is always better than code that requires
> comments.
If you knew C++ well enough, you would be familiar with resources that are
bound to a scope. Things like strings and containers are all bound to some
scope and at the end of the scope the resource is released. Similarly
access to data shared between threads, you acquire the resource and bind
this ownership to the local scope. This principle is a basic part of C++
programming and understanding it means that you don't read the construction
of a lock holder as just a lock operation but also that the lifetime of
that lock is bound to the scope of the lock holder.
The information where a lock is held can thus be read from a single line, if
you understand the underlying principle. If you don't, or are unwilling to
accept it, it will just be a confusingly isolated lock operation without
any according unlock - that's your problem though.
Uli
Daniel James
news:<891602cd-f6d4-4f5e...@f40g2000pri.googlegroups.com
>, David Schwartz wrote:
> > This is just a function of the programmer's familiarity with the
> > RAII idiom. I don't think you can describe it as intrinsically
> > good or evil.
>
> Well, I did. So I can.
I can describe integer arithmetic as evil ... that doesn't mean that it
is so.
> It's intrinsically evil to replace:
>
> l.Unlock();
> }
>
> with:
>
> } // implicitly unlocks 'l'
>
> Self-commenting code is always better than code that requires
> comments.
The only thing evil there is the comment, which says nothing new to the
experienced programmer. It's of the same type as the comment in:
i = i + 1; // increment i
It just adds noise to an already clear construct.
Code such as:
{
ScopedLocker locker();
DoStuff();
}
is self-documenting to anyone who knows (or cares enough to look up)
the meaning of ScopedLocker. It's also exception-safe in that the lock
will be unlocked on exit from the block even if an exception is thrown
by DoStuff(), which is not the case in:
{
UnscopedLock alock;
alock.lock();
DoStuff();
alock.unlock();
}
Failure to use the scoped locking idiom in a language (like C++) that
supports it is just bad, unsafe, error-prone, programming.
Cheers,
Daniel.
Daniel James
> The most compelling reason not to use lock holders as discussed
> up-thread is you can't (at least not without a great deal of effort -
> more than they save) guarantee the resource protected by the lock is
> in a sane state in the event of an exception.
No.
In the case of code like
SomeObject a;
{
ScopedLocker locker();
DoStuff( a );
}
it is the responsibility of DoStuff() itself to ensure that the object
a -- the one protected by the locker -- is in a sane state if an
exception is thrown. That is: DoStuff must offer at least the basic
exception safety guarantee. If DoStuff can return -- either normally or
abnormally by throwing an exception -- an leave a in an unsafe state
your code is broken. Fix that before you worry about how to release
your locks.
That argument is quite orthogonal to any argument about the relative
merits of the above code and:
SomeObject a;
{
UnscopedLock alock;
alock.lock();
try
{
DoStuff( a );
}
catch( const SomeException & e )
{
alock.unlock();
throw;
}
alock.unlock();
}
In particular: there's no merit in suggesting that a call to the
hypothetical a.removeInsanity() could be inserted in the catch block,
because by that time it is too late to rescue a.
Cheers,
Daniel.
Ulrich Eckhardt
> The most compelling reason not to use lock holders as discussed
> up-thread is you can't (at least not without a great deal of effort -
> more than they save) guarantee the resource protected by the lock is in
> a sane state in the event of an exception.
I don't understand this claim. You use a mutex to assure that things are
seen the same way by different threads and not modified in an uncoordinated
way. Assuring that things are in a sane state is something different, that
is typically done by some form of encapsulation. If you can't assure that
code leaves things in a sane state, then it doesn't matter whether the code
is multithreaded or not, then the code is doomed anyway.
Uli
Ian Collins
> In article news:<6r3s6tF...@mid.individual.net>, Ian Collins wrote:
>> The most compelling reason not to use lock holders as discussed
>> up-thread is you can't (at least not without a great deal of effort -
>> more than they save) guarantee the resource protected by the lock is
>> in a sane state in the event of an exception.
>
> No.
>
> In the case of code like
>
> SomeObject a;
>
> {
> ScopedLocker locker();
>
> DoStuff( a );
> }
>
> it is the responsibility of DoStuff() itself to ensure that the object
> a -- the one protected by the locker -- is in a sane state if an
> exception is thrown. That is: DoStuff must offer at least the basic
> exception safety guarantee. If DoStuff can return -- either normally or
> abnormally by throwing an exception -- an leave a in an unsafe state
> your code is broken. Fix that before you worry about how to release
> your locks.
>
SomeObject a;
{
ScopedLocker locker();
DoStuff( a );
DoMoreStuff( a );
DoYetMoreStuff( a );
DoStuff( a );
}
--
Ian Collins
Ulrich Eckhardt
> Daniel James wrote:
[...]
>>That is: DoStuff must offer at least the basic
>> exception safety guarantee. If DoStuff can return -- either normally or
>> abnormally by throwing an exception -- an leave a in an unsafe state
>> your code is broken. Fix that before you worry about how to release
>> your locks.
>>
> Fair enough for a trivial case, but what about
>
> SomeObject a;
>
> {
> ScopedLocker locker();
>
> DoStuff( a );
> DoMoreStuff( a );
> DoYetMoreStuff( a );
> DoStuff( a );
> }
Simple thing: if either functions leaves 'a' in an inconsistent state, I
would call it broken. Actually, it doesn't really matter. If either
function depends on the next one being called but might throw an exception
itself, I think it is to some extent broken. Things like the basic
exception guarantee are not fulfilled if it leaves objects in an
inconsistent state. IOW, broken code, with or without the mutex.
Uli
>
>
JC
Why are you not handling exceptions and cleaning up? Scoped locking
isn't the problem there. Your non exception-safe code is the problem.
Whether or not you use a lock holder vs. a manual lock mutex is
irrelevant to the problem. The true problem is still a design issue
entirely unrelated to using lock holders. Either 1) you did not take
steps to ensure your code was exception safe, or 2) you *mistakenly*
specified that "if DoStuff fails, the application is in an undefined
state". Really, proper exception handling and cleanup is the solution
to that problem, *not* using manual lock mutexes.
If you want to address #1, your code is broken, and what you *should*
have done was this:
void CleanupPartiallyDoneStuff (A&) throw ();
try {
ScopedLocker locker();
DoStuff( a );
DoMoreStuff( a );
DoYetMoreStuff( a );
DoStuff( a );
} catch (...) {
CleanupPartiallyDoneStuff( a );
throw;
}
If you want to address #2 to solve it, then you need to make sure that
if DoStuff, DoMoreStuff, or DoYetMoreStuff fails, then they do not, in
fact, leave 'a' in an undefined state.
In both cases, whether you use ScopedLocker, or manually lock or
unlock a mutex, you still have a bug, and that bug is poorly defined
application states and failure to handle exceptions properly. That bug
is *not* that you used a ScopedLocker, further more, the use of
ScopedLocker doesn't make that bug more likely to be noticed. It's
irrelevant.
It's not a compelling reason to not use ScopedLocker, it's a
compelling reason to fix your broken code.
Jason
JC
Also it's important to keep in mind that there's virtually zero cost
of using automatic locking instead of explicit locking. The only
"cost" is that it's not immediately clear what's going on to a
programmer that prefers to read an explicit call to "unlock" instead;
but that cost can easily be eliminated entirely with a comment if one
feels the need.
Jason
Chris M. Thomasson
news:VA.000015f...@nospam.aaisp.org...
> In article news:<6r3s6tF...@mid.individual.net>, Ian Collins wrote:
>> The most compelling reason not to use lock holders as discussed
>> up-thread is you can't (at least not without a great deal of effort -
>> more than they save) guarantee the resource protected by the lock is
>> in a sane state in the event of an exception.
>
> No.
>
> In the case of code like
>
> SomeObject a;
>
> {
> ScopedLocker locker();
>
> DoStuff( a );
> }
>
> it is the responsibility of DoStuff() itself to ensure that the object
> a -- the one protected by the locker -- is in a sane state if an
> exception is thrown. That is: DoStuff must offer at least the basic
> exception safety guarantee. If DoStuff can return -- either normally or
> abnormally by throwing an exception -- an leave a in an unsafe state
> your code is broken. Fix that before you worry about how to release
> your locks.
[...]
Perhaps way off topic wrt this thread but, FWIW, leaving shared
data-structures in an inconsistent state must be expected wrt interprocess
mutexs. If process A is forcefully terminated while it holds a process-wide
mutex, well, what happens to the data? Its inconsistent by default. However,
one can perform a validation of shared state and "fix" any errors therein.
There are many methods, and the techniques probably belong under another
topic...
Well, I remember trying to do a generic robust mutex which wraps OS
process-wide mutex. IIRC, it kind of went something like the following
pseudo-code (for windows):
class robust_mutex {
HANDLE m_handle; // CreateMutex(...)
LONG m_level; // = 0
public:
void lock(bool (*fp_attempt_recover) (LONG, void*), void* state) throw() {
DWORD CONST status = WaitForSingleObject(m_handle, INFINITE);
if (status == WAIT_OBJECT_0) {
if (! InterlockedExchange(&m_level, 0)) {
assert(false);
std::unexpected();
}
return;
} else if (status != WAIT_ABANDONED) {
assert(false);
std::unexpected();
}
if (! fp_attempt_recover(InterlockedExchange(&m_level, 0), state)) {
assert(false);
std::unexpected();
}
return 0;
}
void unlock() throw() {
if (! ReleaseMutex(m_handle)) {
assert(false);
std::unexpected();
}
}
LONG commit_level() throw() {
return InterlockedExchangeAdd(&m_level, 1);
}
};
You could use it like:
struct my_shared_state {
int m_a;
int m_b;
};
static my_shared_state g_my_shared_state = { 0, 1 };
static robust_mutex g_mutex;
static bool my_shared_state_attempt_recover(LONG level, void* state) {
my_state* const self = (my_state*)state;
switch (level) {
case 0: // screwed at level 0
if (self->m_a != self->m_b - 1) {
self->m_a = self->m_b - 1;
}
case 1: // screwed at level 1
if (self->m_b != self->m_a + 1) {
self->m_b = self->m_a + 1;
}
break;
case 2: // already consistent!
break;
default:
return false;
}
return true;
}
static void per_process_mutate() {
g_mutex.lock(my_shared_state_attempt_recover, &g_my_shared_state);
++g_my_shared_state.m_a;
if (g_mutex.commit_level() != 0) {
assert(false);
std::unexpected();
}
++g_my_shared_state.m_b;
if (g_mutex.commit_level() != 1) {
assert(false);
std::unexpected();
}
g_mutex.unlock();
}
This trivial example happens to have two recovery levels. The recovery
granularity is selected by the programmer on a per critical section basis.
IMVHO, the example shows how one can indeed at least attempt to repair
shared data-structures... Humm...
Chris M. Thomasson
"Chris M. Thomasson" <n...@spam.invalid> wrote in message
news:1hj3l.72342$uS1....@newsfe19.iad...
OOPS! level needs to be initialized to `1'!!!!
Sorry about that ;^(...
> public:
> void lock(bool (*fp_attempt_recover) (LONG, void*), void* state) throw()
> {
> DWORD CONST status = WaitForSingleObject(m_handle, INFINITE);
>
> if (status == WAIT_OBJECT_0) {
> if (! InterlockedExchange(&m_level, 0)) {
> assert(false);
> std::unexpected();
> }
> return;
[...]
JC
> "Daniel James" <wastebas...@nospam.aaisp.org> wrote in message
>
> news:VA.000015f...@nospam.aaisp.org...
>
>
>
> > In article <news:6r3s6tF...@mid.individual.net>, Ian Collins wrote:
> >> The most compelling reason not to use lock holders as discussed
> >> up-thread is you can't (at least not without a great deal of effort -
> >> more than they save) guarantee the resource protected by the lock is
> >> in a sane state in the event of an exception.
>
> > No.
>
> > In the case of code like
>
> > SomeObject a;
>
> > {
> > ScopedLocker locker();
>
> > DoStuff( a );
> > }
>
> > it is the responsibility of DoStuff() itself to ensure that the object
> > a -- the one protected by the locker -- is in a sane state if an
> > exception is thrown. That is: DoStuff must offer at least the basic
> > exception safety guarantee. If DoStuff can return -- either normally or
> > abnormally by throwing an exception -- an leave a in an unsafe state
> > your code is broken. Fix that before you worry about how to release
> > your locks.
>
> [...]
>
> Perhaps way off topic wrt this thread but, FWIW, leaving shared
> data-structures in an inconsistent state must be expected wrt interprocess
> mutexs. If process A is forcefully terminated while it holds a process-wide
> mutex, well, what happens to the data? Its inconsistent by default.
Even so, it is inconsistent regardless of whether or not one has used
a scope-based locking mechanism, and the solution is, of course proper
clean up and state management (which you've demonstrated). Just
pointing this out because, IIRC, the original comment that started
this side thread was something along the lines of "scope-based mutexes
are bad because deadlocking instead of crashing makes it harder to
spot inconsistent state errors".
Jason
Daniel James
news:<eca5f25f-7ee2-4539...@k1g2000prb.googlegroups.com>> > ... what about
> >
> > SomeObject a;
> >
> > {
> > ScopedLocker locker();
> >
> > DoStuff( a );
> > DoMoreStuff( a );
> > DoYetMoreStuff( a );
> > DoStuff( a );
> > }
>
> Why are you not handling exceptions and cleaning up?
The best place to clean up when an exception is thrown is *inside*
DoStuff, DoMoreStuff, etc.. Those functions would then rethrow the
exception for it to be handled at a higher level. If the lower-level
functions are themselves exception-safe then this fragment will
automatically be safe too.
IOW Ian's code looks fine to me as it stands (though I realize that Ian
didn't intend it to).
> Scoped locking isn't the problem there. Your non exception-safe code
> is the problem.
Exactly.
> ... your code is broken, and what you *should* have done was this:
>
> void CleanupPartiallyDoneStuff (A&) throw ();
>
> try {
> ScopedLocker locker();
> DoStuff( a );
> DoMoreStuff( a );
> DoYetMoreStuff( a );
> DoStuff( a );
> } catch (...) {
> CleanupPartiallyDoneStuff( a );
> throw;
> }
That's a possible strategy, but one should stress that it is only a
strategy of last resort to be employed where DoStuff, DoMoreStuff,
etc., are not under our control and are known not to be exception-safe
themselves. In the general case there may be no way to determine, in
that catch block, what operations may have been performed on a when the
exception was thrown nor what operations need to be performed to return
it to a consistent state. Sometimes you can do it, usually you can't.
As you say: The best strategy is for the individual DoXxxStuff
functions to be made to be exception-safe in and of themselves; then
there is no problem.
> It's not a compelling reason to not use ScopedLocker, it's a
> compelling reason to fix your broken code.
Absolutely!
Cheers,
Daniel.
Daniel James
> Perhaps way off topic wrt this thread but, FWIW, leaving shared
> data-structures in an inconsistent state must be expected wrt
> interprocess mutexs.
Interesting point ...
Isn't (one of) the purpose(s) of a mutex guarding access to a shared
object to prevent another thread/process getting access to an object at
any time that its state is inconsistent?
I would say that in general one should not BE mucking about with the
state of a shared object unless one is sure that no other thread will
get hold of it until one has finished. That typically means that one
must lock it /first/ so the issue should not generally arise.
What am I missing here?
> If process A is forcefully terminated while it holds a process-wide
> mutex, well, what happens to the data? Its inconsistent by default.
Well ... that depends on the interpretation of "forcibly terminated". I
would say that that's not a normal state of affairs -- it's more
exceptional than an exception -- and to some extent that if you go
around doing things like that you can expect trouble!
> However, one can perform a validation of shared state and "fix" any
> errors therein.
That may be possible in some cases, but will not be possible in the
general case. Consider something like a shared linked list into which a
new element is being inserted by some process which is 'assassinated'
while the internal pointers are inconsistent ... it's not immediately
obvious how you could write code to 'correct' the list structure.
Of course: if you anticipate the need you can design the list so that
insertion is a transaction that can be rolled back ... but if you're
using std::list (for example) you can't write that cleanup code
portably.
Cheers,
Daniel.
David Schwartz
> David Schwartz wrote:
> >> And then your function exits in an unexpected place, the 'delete' is
> >> never called, the lock never released, and your program malfunctions.
> >> And this might never be caught in testing.
>
> > Quite the reverse, the lock holder object hides this.
>
> Hides what?
Hides the fact that you forgot to release the mutex.
> > It is trivial to
> > create a system that detects this in testing and makes it "just work"
> > in release.
> And it's trivial to create a scoped lock which never leaks. I really
> prefer writing code which I *know* works even without testing, rather
> than relying on testing to tell me if I made a mistake.
The point is, with the scoped lock, nothing will tell you that you
made a mistake. Everything will be just fine until it unexpectedly
deadlocks.
> > (For example, consider a *proper* lock holder object that
> > asserts if its destructor is called with the lock held.)
> If I was forced at gunpoint to use such a lock, I would write a
> wrapper class for it, which automatically releases the lock when the
> function is exited. Problem solved, no leaks possible.
No leaks are possible anyway, since it unlocks automatically in
release mode.
> I really don't buy this whole "let's deliberately write unsafe code
> because, you know, if it fails it might indicate that our program sucks".
I have no idea what you are talking about. What do you mean by
"unsafe"? My code does the same thing yours does in a release build.
In a debug build, mine will catch mistakes and yours will hide them.
DS
Ulrich Eckhardt
> [The lock holder object] Hides the fact that you forgot to release
> the mutex.
He didn't forget that. The self-documenting code holds the lock throughout
the function call and releases it when the function is exited. And all that
is coded in a single line of code!
> The point is, with the scoped lock, nothing will tell you that you
> made a mistake.
No mistake made. The critical section is described by means of an object's
lifetime. Binding things to the lifetime of an object is a very powerful
mechanism that is used all over C++, so every C++ programmer can be assumed
to know and understand this.
> Everything will be just fine until it unexpectedly deadlocks.
Deadlocks.... you seem to assume that there are multiple mutexes involved.
However, nobody said this must be so. In my experience, most mutexes are
associated with a resource that is shared between a few threads, like e.g.
a worker thread and one for the UI. A useful design in such a context is
that you wrap the mutex and the shared data structure into an object and
then disallow access to the data without holding the mutex. That means that
in every function you lock the mutex, modify the data accordingly and then
return. Writing the single line
scoped_lock lock(m_mutex);
tells the informed programmer that the mutex is locked throughout the
function, unless explicitly unlocked, and released at the end of the scope.
Unless unlocked anywhere, you can simply forget about the mutex and
concentrate on the code instead. It might depend on the application logic,
but in my experience this covers around 95% of all cases.
Now, talking about deadlocks, where do you see one? How about an example? In
particular, I also don't see how the use of a scoped lock would change
anything there. Deadlocks occur when you lock resources in different order
from different places, how you do that doesn't matter.
Uli
Ulrich Eckhardt
> If you want to address #1[basic exception safety], your code is
> broken, and what you *should* have done was this:
>
> void CleanupPartiallyDoneStuff (A&) throw ();
>
> try {
> ScopedLocker locker();
> DoStuff( a );
> DoMoreStuff( a );
> DoYetMoreStuff( a );
> DoStuff( a );
> } catch (...) {
> CleanupPartiallyDoneStuff( a );
> throw;
> }
Guys, I hate to break the news to you, but
scoped_lock lock();
is a (local) function declaration, not a local object of type scoped_lock
that gets its constructor called without any arguments. :P
Other than that, I don't fully agree with you. There is no reason to call
CleanupPartiallyDoneStuff() when locking a mutex fails, so the scoped lock
should, at the very least, be outside of the try-catch clause.
Uli
JC
You are right. Blame it mostly on careless copy + paste :-)
Jason
Juha Nieminen
> Juha Nieminen wrote:
>> Dave Butenhof wrote:
>>
>>> When this programming error occurs, you'll have a far easier time
>>> isolating and fixing the bug if the failure mode is "deadlock because
>>> the mutex wasn't released" instead of "corrupted data".
>> If your data gets corrupted because the function was exited
>> unexpectedly, then your design sucks.
>>
> So you can guarantee that your data is safe at any point where the lock
> is held? Wow, I'm impressed.
I myself am much more impressed with your proposed "solution" to that
problem: To use manual unlocking.
Not only does manual unlocking make your code less safe, it also
doesn't fix anything. There is absolutely no advantage in manual
unlocking. It can only cause problems.
The argument "it will make it easier to catch problems with unexpected
exceptions" is just ridiculous. You are using a tool for something it's
has never been designed to. And it what about other functions which may
also misbehave in case of thrown exceptions and where mutexes are *not*
used? Are you going to add a mutex to all functions just to make it
"easier to find problems with thrown exceptions"?
If you are using manual mutexes as a tool to find design problems in
your code, you are seriously misusing a tool for a completely wrong
purpose. A mutex is not a debugging tool.
Moreover, in many cases it might be completely *ok* if a function ends
abruptly because of a thrown exception. If it has been properly designed
it will not malfunction. In that case a scoped lock will do exactly what
it should.
> I'm with Dave and David, I used scoped locks many years ago when C++ was
> a new toy and soon learned the error of my ways.
Yes, let's go back to C, where everything must be done manually.
Juha Nieminen
> On Dec 19, 8:30 pm, Juha Nieminen <nos...@thanks.invalid> wrote:
>> David Schwartz wrote:
>>>> And then your function exits in an unexpected place, the 'delete' is
>>>> never called, the lock never released, and your program malfunctions.
>>>> And this might never be caught in testing.
>>> Quite the reverse, the lock holder object hides this.
>> Hides what?
>
> Hides the fact that you forgot to release the mutex.
I didn't forget anything. I said to the compiler "release this when
the function ends", and it's doing exactly what I have told it to do.
How is that forgetting something?
Do you also oppose garbage collection? After all, you are "forgetting"
to release memory and leaving it to the compiler/runtime to clean up
your mess.
>>> It is trivial to
>>> create a system that detects this in testing and makes it "just work"
>>> in release.
>
>> And it's trivial to create a scoped lock which never leaks. I really
>> prefer writing code which I *know* works even without testing, rather
>> than relying on testing to tell me if I made a mistake.
>
> The point is, with the scoped lock, nothing will tell you that you
> made a mistake.
Nothing tells me I made a mistake because there is no mistake. I said
"release the lock when the function exits", and it's doing exactly that.
There is no mistake.
> Everything will be just fine until it unexpectedly
> deadlocks.
Oh, so now manual unlocking will prevent deadlocks better than
automatic unlocking.
>>> (For example, consider a *proper* lock holder object that
>>> asserts if its destructor is called with the lock held.)
>
>> If I was forced at gunpoint to use such a lock, I would write a
>> wrapper class for it, which automatically releases the lock when the
>> function is exited. Problem solved, no leaks possible.
>
> No leaks are possible anyway, since it unlocks automatically in
> release mode.
Now I'm completely confused.
>> I really don't buy this whole "let's deliberately write unsafe code
>> because, you know, if it fails it might indicate that our program sucks".
>
> I have no idea what you are talking about. What do you mean by
> "unsafe"?
Unsafe as in: Can leak.
> My code does the same thing yours does in a release build.
> In a debug build, mine will catch mistakes and yours will hide them.
What mistakes? You keep using that word and I have no idea what you
are talking about.
Besides, mutexes are not a debugging tool. If you are using them to
debug, you are badly misusing a tool for something which they are not
designed for.
Chris Friesen
> Code such as:
>
> {
> ScopedLocker locker();
>
> DoStuff();
> }
>
> is self-documenting to anyone who knows (or cares enough to look up)
> the meaning of ScopedLocker. It's also exception-safe in that the lock
> will be unlocked on exit from the block even if an exception is thrown
> by DoStuff(), which is not the case in:
>
> {
> UnscopedLock alock;
> alock.lock();
>
> DoStuff();
>
> alock.unlock();
> }
>
> Failure to use the scoped locking idiom in a language (like C++) that
> supports it is just bad, unsafe, error-prone, programming.
I have quite a bit of experience with concurrency, but basically none of
it in C++. In the scoped locking case, how do you deal with having a
certain amount of work that logically happens in the same function but
doesn't require the lock to be held? With an explicit unlock this is
simple, but with a scoped lock it seems like you'd need to introduce an
additional scope layer just to handle it.
Whats the usual way of dealing with this? Is there some way to do an
explicit unlock if desired?
Chris
Chris Friesen
> Code such as:
>
> {
> ScopedLocker locker();
>
> DoStuff();
> }
>
> is self-documenting to anyone who knows (or cares enough to look up)
> the meaning of ScopedLocker. It's also exception-safe in that the lock
> will be unlocked on exit from the block even if an exception is thrown
> by DoStuff(), which is not the case in:
>
> {
> UnscopedLock alock;
> alock.lock();
>
> DoStuff();
>
> alock.unlock();
> }
>
> Failure to use the scoped locking idiom in a language (like C++) that
> supports it is just bad, unsafe, error-prone, programming.
I have quite a bit of experience with concurrency, but basically none of
Chris Friesen
> Well ... that depends on the interpretation of "forcibly terminated". I
> would say that that's not a normal state of affairs -- it's more
> exceptional than an exception -- and to some extent that if you go
> around doing things like that you can expect trouble!
There are a number of ways you could get into this scenario, and some of
them aren't even all that exceptional.
Consider an uncorrectable ECC fault that results in a SIGBUS to the
application. Most apps don't handle this signal, so they are
immediately killed. Regular interprocess mutexes are left in an
undetermined state, and any shared data is also in an undetermined stated.
In some cases, a robust mutex will allow another process to detect that
the holder was killed and the data may be in a bad state. In certain
cases, the other process may be able to audit the data and possibly
correct it if necessary.
Chris
Torsten Robitzki
> I have quite a bit of experience with concurrency, but basically none of
> it in C++. In the scoped locking case, how do you deal with having a
> certain amount of work that logically happens in the same function but
> doesn't require the lock to be held? With an explicit unlock this is
> simple, but with a scoped lock it seems like you'd need to introduce an
> additional scope layer just to handle it.
>
> Whats the usual way of dealing with this? Is there some way to do an
> explicit unlock if desired?
If you think, that scope locks do fit your needs best, you can use them.
{
scopelock lock(mutex);
do_a();
}
do_b();
{
scopelock lock(mutex);
do_c();
}
Or you can introduce an unlock guard
scopelock lock(mutex);
do_a();
{
scopeunlock unlock(lock);
do_b();
}
do_c();
But you can also add explicit lock() and unlock() functions to your
scope guard:
scopelock lock(mutex);
do_a();
lock.unlock();
do_b();
lock.lock();
do_c();
The problem here is that you have to be aware that the mutex is not
locked in case that do_b() throws an exception, so if the mutex have to
be locked, while cleanup, you have to lock it again by an exception
handler for example.
But if this is all getting to complicated for a given problem, one can
still fall back to manually locking.
best regards
Torsten
--
kostenlose Wirtschaftssimulation: http://www.financial-rumors.de
Daniel James
> There are a number of ways you could get into this scenario, and
> some of them aren't even all that exceptional.
Ye...es ... for some value of "all that".
> Consider an uncorrectable ECC fault that results in a SIGBUS to the
> application. Most apps don't handle this signal, so they are
> immediately killed. Regular interprocess mutexes are left in an
> undetermined state, and any shared data is also in an undetermined
> stated.
OK, I think that's a valid point ... though I would take issue with a
claim that it wasn't "all that exceptional". Surely the lesson here is
that you SHOULD handle SIGBUS and other such signals in this sort of
multi-tasking code?
Any process that changes the state of a shared object should not
relinquish control of that object while it's state is inconsistent --
and that means that it shouldn't allow itself to be killed off without
making the object's state consistent first.
> In some cases, a robust mutex will allow another process to detect
> that the holder was killed and the data may be in a bad state. In
> certain cases, the other process may be able to audit the data and
> possibly correct it if necessary.
Yes ... "In some cases" ... "may be able to". Agreed. No question.
Surely, though, what we should be striving towards is a way to write
code that won't let itself get into an undefined state in the first
place. That means (in part, at least) that code needs to be exception
safe, and (as you've shown) that it needs to be signal-safe, too.
Cheers,
Daniel.
Daniel James
> In the scoped locking case, how do you deal with having a certain
> amount of work that logically happens in the same function but
> doesn't require the lock to be held? With an explicit unlock this
> is simple, but with a scoped lock it seems like you'd need to
> introduce an additional scope layer just to handle it.
That is exactly what you do do. Fortunately scopes are cheap, and easy
to read.
That's *why* I included a pair of braces in the scoped example I gave,
they define the limit of existence of the ScopedLocker object, and so
the limit of the locking -- the scope of the lock, if you will.
Consider:
void DisplayNumber( SomeObject & a )
{
int number( 0 );
{
ScopedLocker locker();
number = GetNumberFrom( a );
}
output( "The number from a is", number );
}
The inner braces define the scope of the ScopedLocker ... and so the
extent of the lock.
[Of course, in the above 'output' will not be called if 'GetNumberFrom'
throws. If that's not what you want you'll have to catch the exception
in 'DisplayNumber', otherwise you can handle it somewhere else.]
> Is there some way to do an explicit unlock if desired?
That depends on the actual implementation of the scoped locking
object(s) being used. Typically you declare a (global?) lock for the
object being guarded and the ScopedLocker is just a helper that manages
that lock and unlocks it in its (the manager's) destructor. You can
call unlock explicitly on the lock object before the helper goes out of
scope if it is safe to do so.
That is: you can do it if the scoped locker can determine that the lock
has already been unlocked (e.g. the lock object supports an isLocked
method) or if it is harmless to unlock the lock a second time.
IOW you might have something like:
SomeGlobalObject aThing;
SomeMutex aMutex;
...
void DoSomething()
{
ScopedLocker locker( aMutex );
DoStuff( aThing );
if( someCondition )
{
aMutex.unlock();
DoSomethingElse();
}
}
... but what's the point? You can always release the lock by ending
the scope anyway;
void DoSomething()
{
{
ScopedLocker locker( aMutex );
DoStuff( aThing );
}
if( someCondition )
{
DoSomethingElse();
}
}
Cheers,
Daniel.
Daniel James
wrote:
> Guys, I hate to break the news to you, but
>
> scoped_lock lock();
>
> is a (local) function declaration, not a local object of type
> scoped_lock that gets its constructor called without any arguments.
<smile> Yes, of course, you'd be right if we were writing actual C++
.. but this is sorta pseudocode.
What you actually write is something like
scoped_lock lock( someMutex );
where 'someMutex' is the (global?) mutex that gets locked by the
scoped_lock helper and unlocked by its destruction. We've been eliding
that detail without making it clear (which probably doesn't help).
> There is no reason to call CleanupPartiallyDoneStuff() when locking
> a mutex fails, so the scoped lock should, at the very least, be
> outside of the try-catch clause.
Now that *is* a good point ...
.. and, more particularly, you want the lock to be held while
CleanupPartiallyDoneStuff runs so you NEED the scope of the scoped lock
to include the whole of the try AND catch blocks.
Good catch (pun intended)!
Cheers,
Daniel
Chris M. Thomasson
Humm, who would ever feel safe using `std::list' in shared memory anyway?
As for the linked list case... Well, one could do something like the
following; using the contrived `robust_mutex' example I gave in a previous
post:
struct node {
struct node* next;
};
struct list_push {
struct list* list;
struct node* new_node; // cannot dereference!
struct node* head;
};
struct list {
struct node* head; // = 0
robust_mutex mutex;
};
static bool list_push_attempt_recover(LONG level, void* state) {
struct list_push* const self = state;
switch (level) {
case 0:
// Okay, no mutations have been made; we have a consistent list!
break;
case 1:
// Well, we have a valid push record; attempt to fix the list.
if (self->list->head != self->head) {
assert(self->list->head == self->new_node);
// okay, the dead process apparently mutated the head; fix it!
self->list->head = self->head;
}
break;
case 2: // already consistent!
break;
default:
return false;
}
// reset push record
self->list = NULL;
self->new_node = self->head = NULL;
return true;
}
static void list_push(struct list* const self, struct node* const new_node)
{
static struct list_push g_list_push = { NULL };
self->mutex.lock(list_push_attempt_recover, &g_list_push);
// setup the push record!
g_list_push.list = self;
g_list_push.new_node = new_node;
g_list_push.head = new_node->next = self->head;
self->mutex.commit_level();
// okay, push record commited; peform the mutation!
self->head = new_node;
self->mutex.commit_level();
// Bingo, the mutation has been commited! ;^)
self->mutex.unlock();
}
That is a very trivial example of a single linked list that can actually
"repair" prematurely terminated push operations...
Any thoughts?
Chris M. Thomasson
[...]
I forgot to mention that I left out details on how to handle recovery
attempts that are themselves terminated prematurely. It can be done, but it
gets tricky, and not all data-structures are compatible.
Chris M. Thomasson
> Chris Friesen wrote:
>> There are a number of ways you could get into this scenario, and
>> some of them aren't even all that exceptional.
>
> Ye...es ... for some value of "all that".
>
>> Consider an uncorrectable ECC fault that results in a SIGBUS to the
>> application. Most apps don't handle this signal, so they are
>> immediately killed. Regular interprocess mutexes are left in an
>> undetermined state, and any shared data is also in an undetermined
>> stated.
>
> OK, I think that's a valid point ... though I would take issue with a
> claim that it wasn't "all that exceptional". Surely the lesson here is
> that you SHOULD handle SIGBUS and other such signals in this sort of
> multi-tasking code?
> Any process that changes the state of a shared object should not
> relinquish control of that object while it's state is inconsistent --
> and that means that it shouldn't allow itself to be killed off without
> making the object's state consistent first.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Exactly how would you accomplish that? How are you going to ensure that a
user never terminates a process while it holds an inter-process
synchronization object? Also, your daemons/services should not crash if a
user does decides to kill a process that happens to be holding one of its
mutexs. IMVHO, its is usually very hard and tedious to introduce robust
fail-safe features into process-wide applications...
Juha Nieminen
> Is there some way to do an explicit unlock if desired?
Since there is no universal standard for scoped locks in C++ (at least
not yet), it will depend on the actual scoped lock implementation.
If you are writing the scoped lock yourself (to internally use eg.
Pthreads mutexes) it's rather trivial to add some "unlock()" member
function to your scoped lock class if you want.
Usually, though, it's a better idea to use scope blocks.
Ulrich Eckhardt
> I have quite a bit of experience with concurrency, but basically none of
> it in C++. In the scoped locking case, how do you deal with having a
> certain amount of work that logically happens in the same function but
> doesn't require the lock to be held? With an explicit unlock this is
> simple, but with a scoped lock it seems like you'd need to introduce an
> additional scope layer just to handle it.
>
> Whats the usual way of dealing with this? Is there some way to do an
> explicit unlock if desired?
Yes, typically those scoped locks have an unlock function to explicitly
release the mutex before the lifetime ends. Further, they have a lock
function and allow optionally not taking the lock in the constructor, in
case you construct it earlier than you need the lock.
Actually, I have quite some code that goes like this:
scoped_lock lock(configuration_mutex);
string const s1 = configuration.s1;
string const s2 = configuration.s2;
int const i3 = configuration.i3;
lock.unlock();
// Do something with these three objects now
// but leave the configuration unlocked.
The explicit unlock allows me to sleep in the code that follows without
unnecessarily blocking other threads or even causing complete deadlocks.
Still, if e.g. copying the strings caused a bad_alloc to be thrown, the
lock would be correctly released.
However, I don't find these scoped locks are that good either. I much prefer
a combination that you would use like this:
// this holds a 'foo' and a mutex
resource<foo> my_foo;
...
resource<foo>::lock f(my_foo);
f->do_this();
f.unlock();
f->do_that();
Using a lock object would be the only way to access the underlying foo
object, while with a scoped lock and detached mutex its use is still
volontary. In the above code, since you manually released the lock, the
last line will BTW compile but assert() at runtime.
Uli
David Schwartz
> Not only does manual unlocking make your code less safe, it also
> doesn't fix anything. There is absolutely no advantage in manual
> unlocking. It can only cause problems.
How does it make your code less safe? (Remember, my scheme will detect
a missing unlock in release code and unlock the mutex automatically.)
DS
David Schwartz
[snip]
It is obvious that you are replying to some argument that is
completely different from the one I'm making.
DS
Ulrich Eckhardt
Stop being so arrogant, David. The problem here is that you seem to refuse
to accept that someone does simply not consider the behaviour of a scoped
lock an error and you fail to provide evidence as to why it is. E.g. you
claimed it can cause deadlocks, but yet failed to provide the example. You
claimed that it causes unlocks of mutexes with datastructures in an invalid
state, a claim that has been proven (at least as far as I'm concerned) to
be caused by buggy code elsewhere. Now you're attacking Juha on a personal
basis because he fails to understand your faulty reasoning.
Seriously, how about you accept that the definition of a scoped lock means
to some people what it has been expressed to mean and from then on try make
arguments against? The reason I'm asking this is that that is the single
point where opinion and personal taste seem to matter here. Your personal
taste demands not to do that, but from that you derive the claim that they
are technically inferior, which is just faulty reasoning.
think about it
Uli
Daniel James
wrote:
> Exactly how would you accomplish that?
I suspect that answer to that is partly "implementation defined" and
partly "maybe you can't". Most systems provide some sort of signal that
an application can handle even when it's being shut down forcibly and
that (where available) can clearly be used to trigger some sort of
cleanup operation (when necessary) -- it's analogous to exception
handling so shouldn't be rocket-science (where the facilities exist at
all).
> How are you going to ensure that a user never terminates a process
> while it holds an inter-process synchronization object?
You're talking about process assassination rather than any orderly
shutdown. In general users shouldn't do that -- and shouldn't /have/ to
do that ... and (unless they're root/Administrator) probably *can't* do
that. It is not a "normal" event. I can't say that I'm particularly
worried if it occasionally ends in tears.
> IMVHO, its is usually very hard and tedious to introduce robust
> fail-safe features into process-wide applications...
It *shouldn't* be hard -- it certainly shouldn't be as hard as it
actually is in practice -- and it *needn't* be tedious. We're starting
to understand the problems associated with exception (/sensu lato/)
safety and starting to develop idioms and to build support into
libraries and frameworks that make exception-safe programming easier
and less tiresome.
It's also becoming obvious that as per-thread throughput reaches
physically-imposed limits we need to rely more and more on multiple
threading and multiple tasks to achieve high performance and
throughput, and that some of these synchronization problems that have
always seemed a little esoteric and outside the purview of the average
jobbing programmer are going to become bread-and-butter work for us
all.
Cheers,
Daniel.
Daniel James
wrote:
> That is a very trivial example of a single linked list that can actually
> "repair" prematurely terminated push operations...
Granted ...
> Any thoughts?
How about: "Prevention is better than cure"?
Cheers,
Daniel.
rani_s...@hotmail.com
> David Schwartz wrote:
> > On Dec 21, 1:26 pm, Juha Nieminen <nos...@thanks.invalid> wrote:
> > [snip]
>
> > It is obvious that you are replying to some argument that is
> > completely different from the one I'm making.
>
> Stop being so arrogant, David. The problem here is that you seem to refuse
> to accept that someone does simply not consider the behaviour of a scoped
> lock an error and you fail to provide evidence as to why it is. E.g. you
> claimed it can cause deadlocks, but yet failed to provide the example.
Our midsize project with ~50 developers might be helpful case study.
Beside of auto-lock I also introduced a java style scope lock which is
if-style syntactic sugar:
SCOPE_LOCK(mutex)
{
// atomic
}
Developers seem to like the scope lock construct much more than auto-
look.
I'm not sure about the reason but maybe people understand that special
care should be taken when locking and therefore they prefer explicit
yet safe construct that makes the associated block more noticeable.
I also prefer scope-lock over auto-lock since it's more noticeable and
not sensitive for destruction ordering that might be accidental with
auto-lock leading to higher contention and in some cases deadlocks
(e.g. calling external callback in destructor).
OHOH, there are rare cases that require multiple explicit lock/unlock
and therefore our auto-lock also supports explicit lock/unlock.
With better construct like scope-lock it's easier to see the real
problems of locking that are typically doing too much under a lock and
therefore being sensitive to deadlocks (performance is rarely an
issue) or obscure reasoning for serialization.
Rani
Alexander Terekhov
Juha Nieminen wrote:
>
> Ian Collins wrote:
> > Juha Nieminen wrote:
> >> Dave Butenhof wrote:
> >>
> >>> When this programming error occurs, you'll have a far easier time
> >>> isolating and fixing the bug if the failure mode is "deadlock because
> >>> the mutex wasn't released" instead of "corrupted data".
> >> If your data gets corrupted because the function was exited
> >> unexpectedly, then your design sucks.
> >>
> > So you can guarantee that your data is safe at any point where the lock
> > is held? Wow, I'm impressed.
>
> I myself am much more impressed with your proposed "solution" to that
> problem: To use manual unlocking.
There are basically four approaches here:
1. Don't let unlock() happen automatically for unexpected throws
(with/from unexpected reasons/places) and let eventual deadlock (with
lack of higher level heartbeat watchdog, something like EOWNERDEAD or
ENOTRECOVERABLE returned on subsequent lock()) to expose corruption.
(Practical approach.)
2. Pretend that unexpected throws just can't happen under the current
C++ language. (Utopia.)
3. Pretend that unexpected throws must call abort() at throw points. (In
general, utopia as well given failed attempts to get rid of idiotic and
unnecessary catch(...) all across the current C++ language and commonly
known anti-patterns using idiotic and unnecessary catch(...).)
4. Dream of the C++ language really ensuring that unexpected throws just
can't happen and hence there is no problem at all with automatic
unlock(). (Proper Nirvana.)
Where are you coming from, Juha? ;-)
regards,
alexander.
David Schwartz
Ulrich Eckhardt wrote:
> Stop being so arrogant, David. The problem here is that you seem to refuse
> to accept that someone does simply not consider the behaviour of a scoped
> lock an error and you fail to provide evidence as to why it is.
I've provided the evidence at least three times now.
> E.g. you
> claimed it can cause deadlocks, but yet failed to provide the example.
I did provide the example. If it makes you happy here it is again:
void foo(void)
{ // call without the 'bar' lock. call with the 'qux' lock
// tons and tons of code
}
Now, someone looks at this code and doesn't realize that right before
the '}', the 'qux' lock is held. The '}' destroys the scoped lock
holder, releasing it. So he modified the code as follows:
void foo(void)
{ // call without the 'bar' lock. call with the 'qux' lock
// tons and tons of code
function_that_cannot_be_called_with_the_qux_lock_held();
}
Boom, deadlock.
Now, if the original function looked like this:
void foo(void)
{ // call without the 'bar' lock. call with the 'qux' lock
// tons and tons of code
unlock(qux);
}
Now the mistake is almost impossible to make. Anyone adding code
around the '}' must choose whether to place it before or after the
release of the 'qux' lock.
> You
> claimed that it causes unlocks of mutexes with datastructures in an invalid
> state, a claim that has been proven (at least as far as I'm concerned) to
> be caused by buggy code elsewhere.
I never claimed that,
> Now you're attacking Juha on a personal
> basis because he fails to understand your faulty reasoning.
No, because he is criticizing something other than what I'm saying,
despite the fact that I've clarified the exact point he is still
unclear on at least three times.
> Seriously, how about you accept that the definition of a scoped lock means
> to some people what it has been expressed to mean and from then on try make
> arguments against? The reason I'm asking this is that that is the single
> point where opinion and personal taste seem to matter here. Your personal
> taste demands not to do that, but from that you derive the claim that they
> are technically inferior, which is just faulty reasoning.
>
> think about it
It is not an issue of personal taste at all, unless you consider
correct code to be my personal taste. Yes, I have a distaste for buggy
code.
Code must always know what mutexes it holds or might hold. That's
critical for developing correct multi-threaded code. Having '}' unlock
a mutex makes it much more likely that code will be added in the wrong
place, running with the wrong locks held.
And there is no advantage. As I've said at least three times, there
are other ways to implement whatever handling you want in the case
where an 'unlock' is forgotten, including seamlessly releasing the
lock. (I don't think that's usually a good idea, but that is a matter
of taste, and my own scoped locks *do* implement automatic release if
a release is forgotten.)
DS
Anthony Williams
> Chris Friesen wrote:
>> Is there some way to do an explicit unlock if desired?
>
> Since there is no universal standard for scoped locks in C++ (at least
> not yet), it will depend on the actual scoped lock implementation.
The new C++0x standard provides two types. std::lock_guard<std::mutex>
is explicitly scoped --- it always locks in the constructor and
unlocks in the destructor. OTOH, std::unique_lock<std::mutex> can be
constructed without acquiring a lock, and can be locked or unlocked
midway through its lifetime by calling the lock() and unlock() member
functions. If it holds a lock when it is destructed then it unlocks
the mutex.
Anthony
--
Anthony Williams
Author of C++ Concurrency in Action | http://www.manning.com/williams
Custom Software Development | http://www.justsoftwaresolutions.co.uk
Just Software Solutions Ltd, Registered in England, Company Number 5478976.
Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK
David Schwartz
> Our midsize project with ~50 developers might be helpful case study.
> Beside of auto-lock I also introduced a java style scope lock which is
> if-style syntactic sugar:
> SCOPE_LOCK(mutex)
> {
> // atomic
>
> }
>
> Developers seem to like the scope lock construct much more than auto-
> look.
> I'm not sure about the reason but maybe people understand that special
> care should be taken when locking and therefore they prefer explicit
> yet safe construct that makes the associated block more noticeable.
I like it better because the sole purpose of the '}' at the end of the
SCOPE_LOCK is to indicate that it is the end of the lock's scope. This
makes it much harder to confuse it with the 'mere' end of an 'if' or
'while' block.
On the flip side, I don't see the advantage of this structure. And
there is one large disadvantage. If the scope is very large and
contains a 'return' inside it, it may not be clear that the 'return'
is an unlock. This can lead to erroneous code being added around the
'return' statement, inside the block.
For example, if someone sees the 'return' and adds a 'foo();' before
it, it may not be clear to them that they are calling the 'foo'
function while holding the scoped lock. It's too easy to think that
because it is safe to call 'return', the code must be in a state in
which it is safe to return to the calling code. That is, it's very
easy to think that any code that could safely run in the caller can
safely run before the 'return', and that is not so. This is a real
mistake that is not terribly uncommon. Commenting every 'return'
inside a scoped lock would be awful.
DS
Juha Nieminen
> There are basically four approaches here:
>
> 1. Don't let unlock() happen automatically for unexpected throws
> (with/from unexpected reasons/places) and let eventual deadlock (with
> lack of higher level heartbeat watchdog, something like EOWNERDEAD or
> ENOTRECOVERABLE returned on subsequent lock()) to expose corruption.
> (Practical approach.)
>
> 2. Pretend that unexpected throws just can't happen under the current
> C++ language. (Utopia.)
>
> 3. Pretend that unexpected throws must call abort() at throw points. (In
> general, utopia as well given failed attempts to get rid of idiotic and
> unnecessary catch(...) all across the current C++ language and commonly
> known anti-patterns using idiotic and unnecessary catch(...).)
>
> 4. Dream of the C++ language really ensuring that unexpected throws just
> can't happen and hence there is no problem at all with automatic
> unlock(). (Proper Nirvana.)
You forgot the fifth approach:
5. Expect that exceptions might be thrown at any point and make it so
that the function will release the lock if that happens.
A thrown exception may not be a fatal error per se, and it may well be
an exception handling of which is not a duty of this function at all
(eg. out of memory exceptions). There's no reason why this function
cannot simply *work correctly* regardless of whether something throws an
exception in the middle of it or not.
For the function to work correctly, if it locks a mutex, it has to
unlock it before exiting. You have two choices for this:
1) Enclose everything in the function inside a 'try' block, catch
everything, and in the 'catch' block release the lock and rethrow.
2) Use a scoped lock, which achieves the exact same thing with less
writing, in a less error-prone way.
The advantage of method 2 is that it will also work with all explicit
function exits without having to manually unlock before each.
Juha Nieminen
> I did provide the example. If it makes you happy here it is again:
>
> void foo(void)
> { // call without the 'bar' lock. call with the 'qux' lock
> // tons and tons of code
> }
>
> Now, someone looks at this code and doesn't realize that right before
> the '}', the 'qux' lock is held. The '}' destroys the scoped lock
> holder, releasing it. So he modified the code as follows:
>
> void foo(void)
> { // call without the 'bar' lock. call with the 'qux' lock
> // tons and tons of code
> function_that_cannot_be_called_with_the_qux_lock_held();
> }
>
> Boom, deadlock.
Basically you are saying that since the function was poorly commented,
and someone might not read and try to understand the contents of the
function before adding code to it, you should change to manual unlocking.
If someone does that, then he is obviously beyond the help of the
self-commenting code that manual unlocking adds.
And if you are really so worried about the "}" not telling that it
unlocks the mutex, wouldn't it be much less trouble to *add a comment*
there saying so, rather than going through all the trouble that manual
unlocking causes?
David Schwartz
> Basically you are saying that since the function was poorly commented,
> and someone might not read and try to understand the contents of the
> function before adding code to it, you should change to manual unlocking.
It's not that it's poorly commented. It's not possible to fix it with
comments. At least, it's not realistically possible to fix it with
comments.
> If someone does that, then he is obviously beyond the help of the
> self-commenting code that manual unlocking adds.
How do you figure?
> And if you are really so worried about the "}" not telling that it
> unlocks the mutex, wouldn't it be much less trouble to *add a comment*
> there saying so, rather than going through all the trouble that manual
> unlocking causes?
What trouble does manual unlocking cause? It causes no trouble at all.
And if you're going to put comments everywhere you automatically
unlock, what's the point of automatically unlocking? Better to do it
manually and the code is then self-commenting.
DS
Ian Collins
If you get an out of memory exception part way through an update you are
in big trouble and a scoped lock can cause more harm than good.
In my experience, most errors that can occur inside a lock have to be
fixed before the lock is released. Maybe it's just my style, but I
typically use a lock to make a sequence of actions atomic. If one of
those actions fails, I want to roll back the change. To do this I do
use a scoped lock, but the destructor action is to verify the
transaction has been committed. What happens if the transaction has not
been committed depends on the situation. If a roll back is possible,
then roll back, release lock and re-throw is OK. If not, an abort may
may be in order.
> For the function to work correctly, if it locks a mutex, it has to
> unlock it before exiting. You have two choices for this:
>
That depends on the severity of the error, you can't generalise.
> 1) Enclose everything in the function inside a 'try' block, catch
> everything, and in the 'catch' block release the lock and rethrow.
>
> 2) Use a scoped lock, which achieves the exact same thing with less
> writing, in a less error-prone way.
>
> The advantage of method 2 is that it will also work with all explicit
> function exits without having to manually unlock before each.
I still think early returns should be handled differently from
exceptions. I'm very cautious of automatically unlocking on an early
return. If the case for the early return has been correctly thought
through, there's no good reason not to include an unlock in that
execution path.
It's too easy to use a scoped lock as a band-aid for a poor design.
They do have their place, but the aren't the silver bullet some people
make them out to be.
--
Ian Collins
Ian Collins
> David Schwartz wrote:
>> I did provide the example. If it makes you happy here it is again:
>>
>> void foo(void)
>> { // call without the 'bar' lock. call with the 'qux' lock
>> // tons and tons of code
>> }
>>
>> Now, someone looks at this code and doesn't realize that right before
>> the '}', the 'qux' lock is held. The '}' destroys the scoped lock
>> holder, releasing it. So he modified the code as follows:
>>
>> void foo(void)
>> { // call without the 'bar' lock. call with the 'qux' lock
>> // tons and tons of code
>> function_that_cannot_be_called_with_the_qux_lock_held();
>> }
>>
>> Boom, deadlock.
>
> Basically you are saying that since the function was poorly commented,
> and someone might not read and try to understand the contents of the
> function before adding code to it, you should change to manual unlocking.
>
--
Ian Collins
David Barrett-Lennard
> What trouble does manual unlocking cause? It causes no trouble at all.
I disagree. It increases the amount of code and therefore *reduces*
clarity. I find it remarkable that you consider manual unlocking
increases clarity. As an analogy would you say a language that uses
explicit 'endif', 'endfor', 'endwhile' statements increases clarity?
This of course is a matter of personal taste, and I tend towards the
idea that the most important way to achieve clarity is to make the
code succinct.
One needs a good reason to bloat the code without increasing
information content. I don't believe you have offered any. Your
deadlock example seems silly to me because it should be *obvious* to
the reader that the lock is held in a well defined lexical scope (by
definition). You say automatic locks makes it hard to know where
locks are released whereas I think it makes it trivially easy.
If the block of code is so large that the reader can't remember what
locks have been taken then some restructuring is in order. Making the
code even more verbose with manual locking is an ineffective way to
combat complexity.
A lexically scoped lock is a declarative construct - in the sense that
it only declares that a lock must be taken in the given scope, which
contrasts with explicit imperative statements to gain a lock and later
release the lock. I personally prefer a declarative construct because
it tends to relate more directly to a formal proof of correctness of
the code.
Your idea that the destructor in debug asserts that unlock was called
only seems to be related to ensuring that the relevant block of code
has a no-throw specification. By all means assert this in debug
builds. However that is orthogonal to the concept of a lexically
scoped lock. Eg
class DeclareNoThrow
{
public:
DeclareNoThrow() : reachedEnd(false) {}
~DeclareNoThrow() { assert(reachedEnd); }
void ReachedEnd() { reachedEnd = true; }
private:
bool reachedEnd;
};
#define ASSERT_NO_THROW_BEGIN DeclareNoThrow noThrow;
#define ASSERT_NO_THROW_END noThrow.ReachedEnd();
void foo()
{
ScopedLock lock(myMutex);
ASSERT_NO_THROW_BEGIN
< code with a no-throw spec >
ASSERT_NO_THROW_END
}
David Schwartz
wrote:
> On Dec 24, 8:31 am, David Schwartz <dav...@webmaster.com> wrote:
> > What trouble does manual unlocking cause? It causes no trouble at all.
> I disagree. It increases the amount of code and therefore *reduces*
> clarity. I find it remarkable that you consider manual unlocking
> increases clarity. As an analogy would you say a language that uses
> explicit 'endif', 'endfor', 'endwhile' statements increases clarity?
Yeah, I would say it increases clarity. It also helps catch mistakes
where the ends are mismatched, which does happen from time to time.
I'm not sure it would be a huge increase in clarity, but yes, it would
be.
> This of course is a matter of personal taste, and I tend towards the
> idea that the most important way to achieve clarity is to make the
> code succinct.
I think the most important way to achieve clarity is to make the code
precisely state what it actually does.
> One needs a good reason to bloat the code without increasing
> information content. I don't believe you have offered any. Your
> deadlock example seems silly to me because it should be *obvious* to
> the reader that the lock is held in a well defined lexical scope (by
> definition). You say automatic locks makes it hard to know where
> locks are released whereas I think it makes it trivially easy.
It should be, but the fact is that it isn't always.
> Your idea that the destructor in debug asserts that unlock was called
> only seems to be related to ensuring that the relevant block of code
> has a no-throw specification. By all means assert this in debug
> builds. However that is orthogonal to the concept of a lexically
> scoped lock. Eg
[snip]
If you throw an exception in a case where you broke invariants and the
lock is automatically release, you are screwed. If the invariants do
hold, there is no reason to continue to hold the lock. In other words,
if an exception occurs while you are holding a lock, either your code
was broken to be begin with or you are totally screwed.
There are exceptions, this is not a hard-and-fast rule. But the use of
lexically-scoped locks does encourage people to write code that is
broken in this way. That's not really the fault of the lexically-
scoped locks, but it is a good reason to discourage new programmers
from using them. Lexically-scoped locks let people forget that they
have to choose the precise point where every lock is unlocked. By
hiding the choice, you are less likely to get it right.
DS
David Barrett-Lennard
> On Dec 23, 7:23 pm, David Barrett-Lennard <davi...@iinet.net.au>
> wrote:
>
> > On Dec 24, 8:31 am, David Schwartz <dav...@webmaster.com> wrote:
> > > What trouble does manual unlocking cause? It causes no trouble at all.
> > I disagree. It increases the amount of code and therefore *reduces*
> > clarity. I find it remarkable that you consider manual unlocking
> > increases clarity. As an analogy would you say a language that uses
> > explicit 'endif', 'endfor', 'endwhile' statements increases clarity?
>
> Yeah, I would say it increases clarity. It also helps catch mistakes
> where the ends are mismatched, which does happen from time to time.
> I'm not sure it would be a huge increase in clarity, but yes, it would
> be.
I think it is hideous, and rather analogous to the following Cobol:
ADD YEARS TO AGE.
MULTIPLY PRICE BY QUANTITY GIVING COST.
SUBTRACT DISCOUNT FROM COST GIVING FINAL-COST.
Some folks prefer it because it is "easier to read".
I'd like to see computer languages tend towards the same clarity that
mathematicians have achieved over the centuries.
> > This of course is a matter of personal taste, and I tend towards the
> > idea that the most important way to achieve clarity is to make the
> > code succinct.
>
> I think the most important way to achieve clarity is to make the code
> precisely state what it actually does.
Being declarative, a lexically scoped lock does that very well.
Consider a maintenance programmer is reviewing some code where a mutex
isn't unlocked on every return path from a function. Is this a simple
oversight by the original programmer or is there some subtle reason
why the lifetime of the lock is dynamic? The maintenance programmer
cannot know without more information. Unfortunately most programmers
don't always write adequate documentation so the intent has to be
reverse engineered from the code.
This problem doesn't occur where lexically scoped locks are used where
possible.
> > One needs a good reason to bloat the code without increasing
> > information content. I don't believe you have offered any. Your
> > deadlock example seems silly to me because it should be *obvious* to
> > the reader that the lock is held in a well defined lexical scope (by
> > definition). You say automatic locks makes it hard to know where
> > locks are released whereas I think it makes it trivially easy.
>
> It should be, but the fact is that it isn't always.
>
> > Your idea that the destructor in debug asserts that unlock was called
> > only seems to be related to ensuring that the relevant block of code
> > has a no-throw specification. By all means assert this in debug
> > builds. However that is orthogonal to the concept of a lexically
> > scoped lock. Eg
>
> [snip]
>
> If you throw an exception in a case where you broke invariants and the
> lock is automatically release, you are screwed.
You are screwed because you broke invariants. Not because the lock
was automatically released.
> If the invariants do
> hold, there is no reason to continue to hold the lock. In other words,
> if an exception occurs while you are holding a lock, either your code
> was broken to be begin with or you are totally screwed.
Many C++ programmers use exceptions for error conditions that are not
actually all that exceptional. There is nothing innately wrong with
having lexically scoped locks released automatically when exceptions
are thrown.
> There are exceptions, this is not a hard-and-fast rule. But the use of
> lexically-scoped locks does encourage people to write code that is
> broken in this way. That's not really the fault of the lexically-
> scoped locks, but it is a good reason to discourage new programmers
> from using them. Lexically-scoped locks let people forget that they
> have to choose the precise point where every lock is unlocked. By
> hiding the choice, you are less likely to get it right.
The choice is still there - the lexical scope. Braces can be use to
control that as required. New programmers should be aware that it is
often a good idea to make the scope of a lock as small as possible.
The same argument could be used for an 'endif' statement to make sure
the programmer "gets it right". The counter argument is that the
verbose source code makes the reader less likely to see the relevant
patterns due to increased visual complexity, increasing the chance of
bugs.
Torsten Robitzki
> Juha Nieminen wrote:
>
>>Ian Collins wrote:
>>>So you can guarantee that your data is safe at any point where the lock
>>>is held? Wow, I'm impressed.
>>
>> I myself am much more impressed with your proposed "solution" to that
>>problem: To use manual unlocking.
>
>
> There are basically four approaches here:
> 2. Pretend that unexpected throws just can't happen under the current
> C++ language. (Utopia.)
>
> 3. Pretend that unexpected throws must call abort() at throw points. (In
> general, utopia as well given failed attempts to get rid of idiotic and
> unnecessary catch(...) all across the current C++ language and commonly
> known anti-patterns using idiotic and unnecessary catch(...).)
As we talk about C++ I wonder what could unexpected throws be. If we are
talking about C++ exceptions from a function that has the no throw
guaranty (throw()), than an exception from such a function should result
in a call to std::unexpected() and thus in calling abort(). If the
compiler doesn't support throw lists or implement them so poorly, that
they are not usable and a function that documents to not throw an
exception, throws an exception, than there is a bug in the function,
that throws the exception.
If you are talking about compilers that translate OS
exceptions/traps/signals into C++ exceptions like early MS compilers did
for division be zero, or access violations that that's a totally
different thing and for newer compilers there should be at least a
switch to turn off such broken behavior.
best regards and happy Xmas
Torsten
--
kostenlose Wirtschaftssimulation: http://www.financial-rumors.de
David Schwartz
wrote:
> Consider a maintenance programmer is reviewing some code where a mutex
> isn't unlocked on every return path from a function. Is this a simple
> oversight by the original programmer or is there some subtle reason
> why the lifetime of the lock is dynamic? The maintenance programmer
> cannot know without more information. Unfortunately most programmers
> don't always write adequate documentation so the intent has to be
> reverse engineered from the code.
I already showed you how to solve this problem. In fact, I showed you
a way to automatically detect it in debug builds and fix it in release
builds. (Assuming that releasing the mutex is the correct fix.)
> This problem doesn't occur where lexically scoped locks are used where
> possible.
Of course, but you get in exchange precisely comparable problems.
Suppose the lock is not supposed to be released, but the scoped lock
releases it automatically. Maybe your scoped lock has a 'do not unlock
on destructor' function. Maybe the programmer forgot to call it.
> > > Your idea that the destructor in debug asserts that unlock was called
> > > only seems to be related to ensuring that the relevant block of code
> > > has a no-throw specification. By all means assert this in debug
> > > builds. However that is orthogonal to the concept of a lexically
> > > scoped lock. Eg
You seem to have missed the point that this allows your code to
declare that the lock is always supposed to be released by the time
the block is left. That is, it preserves any self-documenting that
scoped locks have.
> > If you throw an exception in a case where you broke invariants and the
> > lock is automatically release, you are screwed.
> You are screwed because you broke invariants. Not because the lock
> was automatically released.
No, you're fine if the invariants are broken so long as the lock is
still held. It is perfectly valid to break invariants so long as you
do not release the lock. That's, in fact, what locks are for.
> The choice is still there - the lexical scope. Braces can be use to
> control that as required. New programmers should be aware that it is
> often a good idea to make the scope of a lock as small as possible.
Right, but that's unduly difficult with scoped locks. Consider an 'if'
that needs to be inside the scope where one tree of the 'if' has lots
of code that does not need the lock. The tendency will be to simply
hold the lock through the tree. (Again, this is not the fault of
scoped locks. But it's a good reason their use should be discouraged
while people are learning how to use locks properly.)
> The same argument could be used for an 'endif' statement to make sure
> the programmer "gets it right". The counter argument is that the
> verbose source code makes the reader less likely to see the relevant
> patterns due to increased visual complexity, increasing the chance of
> bugs.
Nevertheless, it is vital to know precisely where a lock is released.
There is very little code that can run either with or without a lock,
in most cases, you *must* know. So either self-documenting code or a
comment is essential or problems are likely. Unlocking a mutex is
simply not analogous.
DS
Ulrich Eckhardt
> Ulrich Eckhardt wrote:
>
>> Stop being so arrogant, David. The problem here is that you seem to
>> refuse to accept that someone does simply not consider the behaviour of a
>> scoped lock an error and you fail to provide evidence as to why it is.
>
> I've provided the evidence at least three times now.
Actually, no. You have only shown that people can make mistakes. You have
further raised the claim that scoped locks make it more likely, which IMO
is tightly coupled with the lack of understanding for the full meaning of a
scoped lock that you show.
>> E.g. you
>> claimed it can cause deadlocks, but yet failed to provide the example.
>
> I did provide the example. If it makes you happy here it is again:
>
> void foo(void)
> { // call without the 'bar' lock. call with the 'qux' lock
> // tons and tons of code
> }
>
> Now, someone looks at this code and doesn't realize that right before
> the '}', the 'qux' lock is held.
See, exactly that doesn't happen. If they read this, they will come across
the scoped lock and know immediately that the lock is bound to the object's
lifetime. Well, they will see that provided they understand fully what this
scoped lock means. You for one don't understand it or, rather, refuse to
accept it, so you will also fail to understand the code. Since the concept
is clear and can easily be understood, that is not a problem with the code
though.
> The '}' destroys the scoped lock
> holder, releasing it. So he modified the code as follows:
>
> void foo(void)
> { // call without the 'bar' lock. call with the 'qux' lock
> // tons and tons of code
> function_that_cannot_be_called_with_the_qux_lock_held();
> }
>
> Boom, deadlock.
>
> Now, if the original function looked like this:
>
> void foo(void)
> { // call without the 'bar' lock. call with the 'qux' lock
> // tons and tons of code
> unlock(qux);
> }
>
> Now the mistake is almost impossible to make. Anyone adding code
> around the '}' must choose whether to place it before or after the
> release of the 'qux' lock.
Sure. If someone like e.g. you simply refuses to read the scoped lock as all
it is, that person is unable to understand the code and when the locks are
held or released. If a person accepts the full meaning of a scoped lock,
the code will be clear to them.
>> Seriously, how about you accept that the definition of a scoped lock
>> means to some people what it has been expressed to mean and from then on
>> try make arguments against? The reason I'm asking this is that that is
>> the single point where opinion and personal taste seem to matter here.
>> Your personal taste demands not to do that, but from that you derive the
>> claim that they are technically inferior, which is just faulty reasoning.
>>
>> think about it
>
> It is not an issue of personal taste at all, unless you consider
> correct code to be my personal taste. Yes, I have a distaste for buggy
> code.
It is your "personal taste" that you refuse to accept that there are people
that interpret a scoped lock differently from you. Basically, you seem
unable or unwilling to accept others' point of view and from that you
create a failure in scoped locks, while it actually is your inability to
accept others' opinion. Then you start measuring others' approaches with
your own bias, but you fail to understand that this bias is simply
incompatible to the context in which the approach was made. That is a
logical fallacy, and not a technical problem with either your bias or the
approach taken by others.
BTW: it's strange that even though you claim to dislike buggy code, you
still cite it as better example. I'm talking about this code here:
> void foo(void)
> { // call without the 'bar' lock. call with the 'qux' lock
> // tons and tons of code
> unlock(qux);
> }
This code only catches one of at least two possible exit points, you simply
failed to handle the case of an exception being thrown or a return in the
middle.
> And there is no advantage.
Less code, not having to hook into each and every one of the multiple exit
points of a function. Are those advantages or not?
Uli
Ulrich Eckhardt
> If you throw an exception in a case where you broke invariants and the
> lock is automatically release, you are screwed.
Please reread the statements made about basic C++ exception safety. Point is
that code that leaves a module with its invariants broken is buggy. With or
without any locks, with or without multithreading, that code is buggy and
no locking strategy will ever fix this.
> If the invariants do hold, there is no reason to continue to hold the
> lock. In other words, if an exception occurs while you are holding a
> lock, either your code was broken to be begin with or you are totally
> screwed.
Nope, totally normal case. You try to add an element to a queue protected by
a mutex. The queue is full, so it throws an exception at you.
> There are exceptions, this is not a hard-and-fast rule.
That "rule" is so soft that it's actually useless. Exceptions might not
occur often (otherwise they would be abused in C++), but they are still a
regular event that has to be handled.
Uli
David Barrett-Lennard
> On Dec 23, 10:08 pm, David Barrett-Lennard <davi...@iinet.net.au>
> wrote:
>
> > Consider a maintenance programmer is reviewing some code where a mutex
> > isn't unlocked on every return path from a function. Is this a simple
> > oversight by the original programmer or is there some subtle reason
> > why the lifetime of the lock is dynamic? The maintenance programmer
> > cannot know without more information. Unfortunately most programmers
> > don't always write adequate documentation so the intent has to be
> > reverse engineered from the code.
>
> I already showed you how to solve this problem. In fact, I showed you
> a way to automatically detect it in debug builds and fix it in release
> builds. (Assuming that releasing the mutex is the correct fix.)
As I see for a given situation the programmer should either use 1) a
lexically scoped lock, or 2) direct calls to lock or unlock the mutex.
The first *always* relates to a single well defined lexical scope and
doesn't permit manual unlocking. The second is more flexible and
(only) recommended when locking a lexical scope is unsatisfactory for
that situation.
These are the purist and simplest building blocks and I don't want to
mess with them. I want my basic constructs to have the simplest
possible mathematical properties. You appear to recommend a bastard
child of the two.
> > This problem doesn't occur where lexically scoped locks are used where
> > possible.
>
> Of course, but you get in exchange precisely comparable problems.
> Suppose the lock is not supposed to be released, but the scoped lock
> releases it automatically. Maybe your scoped lock has a 'do not unlock
> on destructor' function. Maybe the programmer forgot to call it.
>
> > > > Your idea that the destructor in debug asserts that unlock was called
> > > > only seems to be related to ensuring that the relevant block of code
> > > > has a no-throw specification. By all means assert this in debug
> > > > builds. However that is orthogonal to the concept of a lexically
> > > > scoped lock. Eg
>
> You seem to have missed the point that this allows your code to
> declare that the lock is always supposed to be released by the time
> the block is left. That is, it preserves any self-documenting that
> scoped locks have.
Granted but the concept lacks simplicity. What do you call it?
PartialLexicalLockerThatAssertsUnlockHasBeenCalled
> > > If you throw an exception in a case where you broke invariants and the
> > > lock is automatically release, you are screwed.
> > You are screwed because you broke invariants. Not because the lock
> > was automatically released.
>
> No, you're fine if the invariants are broken so long as the lock is
> still held. It is perfectly valid to break invariants so long as you
> do not release the lock. That's, in fact, what locks are for.
>
> > The choice is still there - the lexical scope. Braces can be use to
> > control that as required. New programmers should be aware that it is
> > often a good idea to make the scope of a lock as small as possible.
>
> Right, but that's unduly difficult with scoped locks. Consider an 'if'
> that needs to be inside the scope where one tree of the 'if' has lots
> of code that does not need the lock. The tendency will be to simply
> hold the lock through the tree. (Again, this is not the fault of
> scoped locks. But it's a good reason their use should be discouraged
> while people are learning how to use locks properly.)
If it's unduly difficult then a lexically scoped lock is inappropriate
and I would simply make direct calls to lock/unlock the mutex.
> > The same argument could be used for an 'endif' statement to make sure
> > the programmer "gets it right". The counter argument is that the
> > verbose source code makes the reader less likely to see the relevant
> > patterns due to increased visual complexity, increasing the chance of
> > bugs.
>
> Nevertheless, it is vital to know precisely where a lock is released.
> There is very little code that can run either with or without a lock,
> in most cases, you *must* know. So either self-documenting code or a
> comment is essential or problems are likely. Unlocking a mutex is
> simply not analogous.
A lexically scoped lock is excellent self-documenting code. No
comment is needed at the '}'.
Chris M. Thomasson
news:4v4ok4tolppn025lf...@4ax.com...
> David Schwartz wrote:
>
>> On Dec 19, 12:09 am, Anthony Williams <anthony....@gmail.com> wrote:
>>
>> > On the other hand, if the function is less than
>> > 20 lines or so, you can see whether or not it acquires the lock
>> > because all the code fits on one screenful in your editor.
>>
>> But you can't see where the lock is released. All you'll see is '}'
>> which looks a lot different from 'l.Unlock()'.
>
> This is just a function of the programmer's familiarity with the RAII
> idiom. I don't think you can describe it as intrinsically good or evil.
I personally don't mind using comments for this:
static mutex g_mutex;
void foo() {
{
mutex::guard lock(g_mutex);
// [`g_mutex' is locked];
{
mutex::unguard unlock(g_mutex);
// [`g_mutex' is unlocked];
} // g_mutex.lock();
// [`g_mutex' is locked];
} // g_mutex.unlock();
}
That's fine with me. AFAICT, scoped lock RAII objects are a very convenient
exception-safe technique when used with care. I don't see anything evil with
them, assuming they are being utilized by competent programmers of course...
;^)
Juha Nieminen
> If you throw an exception in a case where you broke invariants and the
> lock is automatically release, you are screwed.
If you throw an exception in a case where you broke invariants and the
lock is *not* automatically released, you are equally screwed. Manual
unlocking does not fix your problems.
(And if you will again argument that the lock can be used to trace the
problem, you are still using the completely wrong tool for that purpose.
A lock is not a debugging tool.)
What automatic unlocking does is make your life easier and the code
clearer, as well as reduce silly mistakes (such as forgetting to unlock
in an early return). And no, this is not hiding anything. If I create a
scoped lock at the beginning of the function, I'm *perfectly* aware that
it will be unlocked at each exit point automatically. It's completely
intentional and I'm using that to my advantage. I wouldn't gain anything
from manual unlocking.
Daniel James
news:<e911a34d-b93e-4181...@q30g2000prq.googlegroups.com
>, David Barrett-Lennard wrote:
> I disagree. It increases the amount of code and therefore *reduces*
> clarity.
I agree with you there.
> As an analogy would you say a language that uses explicit 'endif',
> 'endfor', 'endwhile' statements increases clarity?
I think it does, in that it makes it easier to match up the various
start-of-block tokens with their corresponding end-of-block tokens
(even when the indentation has gone awry) and so makes the code more
readable.
That's not quite germane to this discussion, though, as locking is not
a part of the syntax of the language (at least while we're dicussing
C++). If one could write:
MUTEX m
...
LOCK m
DoStuff();
UNLOCK m;
where LOCK and UNLOCK were elemnts of the language syntax you would not
be able to forget the UNLOCK because the compiler would report an
error. You would then have the clarity ascribed to the explicit
lock/unlock idiom but also the safety achieved by using a ScopedLock
object.
What David Schwartz and others are suggesting is a style of programming
which gives the supposed clarity of explicit locking but cannot give
the safety of block-scoping the lock. ISTMT safety is far more valuable
than any slight increase in readability.
Cheers,
Daniel.
In a language that had syntactic primitives LOCK and UNLOCK
Daniel James
news:<209297bb-6614-43b2...@i18g2000prf.googlegroups.com>
, David Schwartz wrote:
> If you throw an exception in a case where you broke invariants and the
> lock is automatically release, you are screwed.
Yes ... but you are still screwed if the lock is not automatically
released. How you manage the lock isn't important here, if you break
invariants you are screwed.
> In other words, if an exception occurs while you are holding a lock,
> either your code was broken to be begin with or you are totally
> screwed.
No. You must ensure that your code does not leave any object in a
non-stable state in the event that an exception is thrown. Unless you do
that you will break invariants and you are screwed. Having ensured that
no invariants will be broken it does no harm to release the lock
(explicitly or using a scoped lock). To fail to release the lock in this
case would be an error (and might well cause a deadlock elsewhere).
It doesn't matter at all, for the purposes of this argument, whether the
lock is released explicitly or using a scoped lock object ... it's just
easier, simpler, more reliable, and clearer to use the scoped object.
> There are exceptions, this is not a hard-and-fast rule. But the use of
> lexically-scoped locks does encourage people to write code that is
> broken in this way.
No it doesn't. A failure to understand the exception mechanism and the
requirements of exception safety lead to people writing code that is
broken in this way. How locks are managed is orthogonal to the problem.
> Lexically-scoped locks let people forget that they
> have to choose the precise point where every lock is unlocked. By
> hiding the choice, you are less likely to get it right.
I tend to disagree. IME people are quite good at failing to understand
where they need to unlock however they're managing their locks ... if
anything I'd say that the notion that the lock is tied to the block
structure of the program helps them to get it right as block structure
is something that they seem to find easier to understand.
Either way, you need to understand what a lock does and when you need
one in order not to write broken code. That's much harder than learning
to use either the explicit locking idiom or scoped locks.
Cheers,
Daniel.
Daniel James
news:<f6b6c30c-7ec9-48e9...@w1g2000prm.googlegroups.com>,
> Maybe your scoped lock has a 'do not unlock on destructor' function.
Don't de daft! It wouldn't be a scoped lock, then, would it?
There may be other kinds of lock-management objects that would allow you
to do that, but one shouldn't call them scoped locks if they don't
manage locks according to a lexical scope.
.. and to think that you were the one who was harping on about clarity
and self-documenting code!
> > > If you throw an exception in a case where you broke invariants
> > > and the lock is automatically release, you are screwed.
>
> > You are screwed because you broke invariants. Not because the lock
> > was automatically released.
>
> No, you're fine if the invariants are broken so long as the lock is
> still held. It is perfectly valid to break invariants so long as you
> do not release the lock. That's, in fact, what locks are for.
How is it going to help that you still hold a lock that you can now
never release? An error has occurred in code that broke some invariant
and that error wasn't handled locally -- and the invariant restored --
so the code is broken. End of story. This has nothing to do with locks.
You would have the code maintain the lock forever to prevent other
threads from using the data it protects -- which isn't the answer:
you'll prevent other code from breaking when it tries to use those data,
but in doing so you will cause deadlocks because the lock cannot be
released and the other code will still break.
If you throw an exception in a case where you broke invariants you
*MUST* restore those invariants before allowing the exception to
propagate. Yes, you need to keep the object locked while you do that,
but once you've done it and you allow the exception to unwind you no
longer need the lock (at least, you do not need it any more than you
would have if your code had completed without the error that caused the
exception) so it is perfectly all right for the scoped lock object to
release the lock at the normal time.
It *MUST* be OK for the scoped lock to do this, and your code *MUST* be
written in such a way that it is so. Otherwise your code is broken --
and it would be broken even in an unapologetically singly-threaded
scenario without a lock in sight.
Cheers,
Daniel.
Daniel James
> It's too easy to use a scoped lock as a band-aid for a poor design.
I'd say that was actually very hard. If the design is bad then nothing
as simple as a scoped lock will save the whole program from being a
buggy mess.
OTOH scoped locks do simplify the task of managing locks correctly, and
that's an important part of writing correct code.
> They do have their place, but the aren't the silver bullet some people
> make them out to be.
Scoped locks are a silver bullet that kills the werewolf called "failing
to unlock a lock" -- they are, of course, no defence against the vampire
of non-exception-safety or the skeletons in the cupboard of bad design.
That is: They are a tool that achieves one thing well, but doing that
one thing correctly won't save you from other errors in your code.
Cheers,
Daniel.
rajiv.k...@gmail.com
programmers tend to overestimate the scope by being just plain lazy. I
have seen scope extended to cover a deep call stack when there was no
need to and which in fact resulted in serilization that went
undetected for a while.
Cheers, -- Rajiv
Juha Nieminen
> It's too easy to use a scoped lock as a band-aid for a poor design.
Manual locks won't make your design any better. In fact, they won't
have any effect whatsoever. The only thing they will do is force you to
write unnecessary (and error-prone) code.
Do you also oppose things like using std::string or std::vector in
C++? After all, they are scoped as well.
Giancarlo Niccolai
> David Schwartz wrote:
>> It's intrinsically evil to replace:
>>
>> l.Unlock();
>> }
>>
>> with:
>>
>> } // implicitly unlocks 'l'
>>
>> Self-commenting code is always better than code that requires
>> comments.
>
> If you knew C++ well enough, you would be familiar with resources that are
> bound to a scope.
Locking is not a resource. Is a thing you do to other surrounding
(unsuspecting) code.
Scoped locks are not evil by themselves. OTOH, scoped lock encourages
harmful practices in code that needs not to be subject to more harmful
practices than those it is usually already subject too.
I explain.
Suppose you have to log something before return, like in the following code:
bool haveIt()
{
if ( shared_resource == available )
{
Log( "Yay, we did it it!" );
return true;
}
Log( "Nay, we didn't make it" );
return false;
}
An unscoped locking version allows this:
bool haveIt()
{
l.lock();
if ( shared_resource == available )
{
l.unlock();
Log( "Yay, we did it it!" );
return true;
}
l.unlock();
Log( "Nay, we didn't make it" );
return false;
}
As you can see, the lock can be held for a minimal time, without the
need to extend it to the log operation, where the lock is useless and
actually harmful.
Compare that with the two possible solutions with scoped lock:
bool haveIt_full()
{
ScopeLock local_lock_for( l );
if ( shared_resource == available )
{
Log( "Yay, we did it it!" );
return true;
}
Log( "Nay, we didn't make it" );
return false;
}
In this first version, we're very readable, and very inefficient. A
second approach which respects correct lock usage semantics would be:
bool haveIt_short()
{
bool haveIt;
{
ScopeLock local_lock_for( l );
haveIt = shared_resource == available;
}
if ( haveIt )
{
Log( "Yay, we did it it!" );
return true;
}
Log( "Nay, we didn't make it" );
return false;
}
but then again, I can't see how this can be more readable and correct than
bool haveIt_short_once_again()
{
l.lock();
bool haveIt = shared_resource == available;
l.unlock();
if ( haveIt )
{
Log( "Yay, we did it it!" );
return true;
}
Log( "Nay, we didn't make it" );
return false;
}
;=======================
IMHO if you (you in general, as "one") hold a mutex long enough to be
possibly affected by an exception raising, you have a problem. If you
(again, general) think that scoped locks are good because they save an
unlock in catch() blocks, then you should first ask yourself why you
have mutexes around when try/catch firings are possible outcomes.
First, because you're very probably breaking the basic principle that
mutexes must be held for the shortest possible time, that is, the time
needed to coordinate shared states visibility.
Second, because you're saving a possible deadlock for sure, but you're
inviting a much more dreadful problem: the dead patient syndrome.
;=========================
Dead patient syndrome is what happens when you messup too much with
threads without control over what happens behind the scenes.
Consider the following code.
...
ScopeLock very_big_lock( l );
...
try {
shared1 = opThatCanRaise( a );
shared2 = opThatCanRaise( b );
NewDataAvailable();
}
catch( MyException &excp ) {
Log( "Sorry, we're out of luck" );
}
Now, suppose that shared1 is the name of the patient that needs to
undergo a surgery operation next, and shared2 is the surgery name.
If you raise after shared1 is set, you have a perfectly consistent,
fault-free, deadlock-free, ever running program, and perfectly dead patient.
Real cases are much more complex and subtle, and resetting the state of
the shared data you mangle with in the wide locks encouraged by scoped
locking on all those paths, some of which not even known in advance, as
exceptions and automatisms are usually thought for future extensions,
can be troublesome, error prone and generally a big headache.
;==================================
All this to say: the two methods aren't one superior to the other in any
aspect. Both carry risks and advantages. BUT, with scoped locking, risks
are often underestimated and hidden. While they require a better
understanding of the underlying situation, and a deeper mastering of the
multithreading mechanics, I often see them just fed to newbies as
"simpler", "easier" and "safer", and this is HARDLY the case.
Actually, if MT requires you to KNOW WHAT YOU'RE DOING much more than
any other development field in IT, automatisms in MT requires even more
expertize.
Zen, which is very similar to MT programming, has a kohan saying that
the newby don't see the road; the practicer follows the road; the master
don't see the road. If you're a master, any new technique is good, and
you can just follow your inspiration. But encouraging the use of "scoped
lock first" as "just better" to newcomers will earn you (once again, you
in a generic sense) programs that never deadlock, but do a lot of worse
things. After all, a deadlock is easily spotted. A dead patient too, but
it's usually too late.
Bottom line: NEVER confuse "automatic" with "simpler" or "safer" (some
confuses it even with "better"). They are non-overlapping concepts.
Bests,
GN.
Giancarlo Niccolai
Ian Collins ha scritto:
> JC wrote:
>> Really, the *only* compelling reason to not use lock holders is if you
>> are using scopes that don't fit cleanly inside the locking scope, e.g.
>> a function like pthread_cond_wait would be hard to implement using a
>> scope-based lock holder for the mutex instead of just a "raw" mutex.
Not correct. It is wonderfully re-implemented posix-semanitc-like with
scoping in C#.
> The most compelling reason not to use lock holders as discussed
> up-thread is you can't (at least not without a great deal of effort -
> more than they save) guarantee the resource protected by the lock is in
> a sane state in the event of an exception.
I undersign this (cfr. the dead patient syndrome I talked about).
>
> By all means use lock holders, but use the destructor to assert the lock
> has been released not to release the lock.
>
I take that "by all means" is meant in the sense: "please, do so if you
prefer, there is no real reason not to do it, if you do it in the proper
way". Correct?
Bests,
GN.
Giancarlo Niccolai
This question has the same relevance and significance as asking if one
should oppose war on cancer because he thinks that war is bad.
Also and besides, please notice that those that are opposing your
position here are not opposing the usage of scoped locking at all; they
are opposing just your positions, which, to my best understanding, are
the followings:
1) scoped locking is always good or better than the other ways of doing
locking.
2) all you have to do about multithreading safety is that of releasing
mutexes before returning from functions.
I am sorry to say that both this points are incorrect. If those two
points doesn't match your position, please clarify it as this is what
emerges from this thread.
Bests,
GN.
Giancarlo Niccolai
>
> More accurately, I should say, it ensured a resource was left in a
> well-defined state when a function exited for any reason -- and that's
> what you should do for *all* resources. If the postconditions of a
> function are:
>
> - Shared object A is in some well-defined state.
> - Mutex M is released.
>
> And the function throws an exception, leaving shared object A in an
> undefined state but with mutex M released, where is the problem? It's
> not in the lock guard, that's the part that *worked*.
See the dead patient syndrome. When you do a mistake about shared object
being left in a non-well defined state:
- if you don't unlock the mutex, the state of the object stays non-well
defined, but no other part of the program can use it. If it tries, it
will deadlock, and your program will eventually halt without any
possible error propagation.
- if you unlock the mutex, the rest of the program may think that the
object is in a well defined state and use it. What happens then depends
on many things, but it ranges from simple data corruption to disaster.
So, even if I agree that you have at least one part over two that
*worked* in the second case, I must dissent on the fact that having one
part working over two possible failing is *always* better than having
both failing.
In this particular case, having a double failure is usually better and
less risky.
Bests,
GN