Boost logo

Boost :

From: williamkempf_at_[hidden]
Date: 2001-03-17 10:17:40

--- In boost_at_y..., Jesse Jones <jesjones_at_h...> wrote:
> >--- In boost_at_y..., Jesse Jones <jejones_at_a...> wrote:
> >> I'm far from a threading expert, but I did take a quick look at
> >code
> >> and the docs. It looks like a good start, but I do have some
> >quibbles:
> >>
> >> 1) You should throw an exception if CreateSemaphore returns nil
> >
> >Other's have pointed out the lack of error handling. I did this
> >intentionally in the draft, but it's something that needs
> >before submission.
> I didn't see mention of the error handling on the Windows side, but
> may well have slipped by me.

The underlying API used means little. If errors can occur using it
you should either handle them or propogate them up through the higher
level API.

> >> 3) Are there examples where having semaphore::up() return false
> >the
> >> semaphore is already at the max count is helpful? In general I
> >don't like
> >> this sort of thing since return codes are way too easy to ignore
> >C. It
> >> also makes writing the Windows implementation dicier since you
> >really want
> >> to distinguish between failing due to max count and due to
> >something like a
> >> corrupted object (the ReleaseSemaphore documentation in MSDN
> >> provide much guidance in how to do this).
> >
> >I can't give you specific examples, but this is the behavior one
> >would expect.
> Well we need more sample code anyway and I sure would like to see
> example where this behavior is better than simply throwing.

I guess I'm not convinced that this action puts us into
an "exceptional state". This particular error is in that gray area
where I think every developer is going to see it in a different
light. I'll have to think about this one, and you're quite correct,
examples would help to evaluate it.
> >Detecting the proper error condition in Win32 should
> >be trivial.
> You would think so and it might even be so. But the
> documentation in MSDN doesn't mention what error codes are returned
> you might have to go by the error your system is returning which
> sound too good.

This is one of those cases where you have to "follow the bouncing
ball", even when the ball goes behind that wall making it awfully
hard to see ;). There's a well defined error returned from
GetLastError() in this case. The code that I'll u/l later this
weekend handles this case if you're curious.
> >> 7) I don't think the locker classes should themselves throw
> >exceptions. The
> >> only way to get this to happen is for the programmer to do
> >something nasty
> >> like call unlock too many times. But this is a straight forward
> >programmer
> >> error, not a runtime error, so it seems like an assert is the
> >answer.
> >
> >Simple errors can, and will occur:
> >
> >boost::mutex::lock lock(mutex);
> >// This code added in maintenance
> >if (something)
> >{
> > // ...
> > lock.unlock();
> >}
> >// ...
> >lock.unlock(); // error!
> >
> >Yes, these are programmer errors, but they are serious errors that
> >result in exceptional conditions.
> Most programmer errors are "serious errors that result in
> conditions".

That I don't agree with. A very large number of errors do not rise
to the point of "exceptional condition". The difficulty is that
often developers disagree on specific errors, which I think is the
case here. So, let me defend my stance a little better.

The errors in question if not detected will result in deadlocks or
race conditions, both of which can be VERY difficult to diagnose.
Asserts are not valid here (the error still exists in Release builds,
and there's no gaurantee that a Debug build will ever be done, or
even if they are, that a specific path is tested under Debug). Error
return codes are often (usually) ignored by programmers. Exceptions
exist in both Debug and Release, can not be ignored, and give a good
indication of what went wrong and where.

> > I think it's appropriate to throw
> >here, and if you don't throw you'll have to instead return an
> >An assert is not enough.
> IMO returning an error code is the worst of the three alternatives.
> assert arguably is enough. The only times it wouldn't be enough is
> those developers foolish enough to run without assert enabled or
> code path was never tested before the software was released. Either
> these may happen, but there's only so much we can do to protect
> developers from themselves and I'm unhappy about making everyone
pay just
> so we can coddle developers who don't use asserts and have lousy QA.

For many libraries I might agree. If you've done ANY threaded
development, however, you know that it's nearly impossible to
reproduce every possible code path (because of race conditions). You
can not rely 100% on a Debugging session to catch all of your errors
here. Remember that this is one of the highest ranked goals of the
Boost.Threads library... safety.
> >> 8) It's a little odd that you don't allow clients to call
> >> basic_lock::lock() twice in a row since the mutex would catch
> >problems.
> >> Admittedly this would be a weird thing to do, but the whole
point of
> >> allowing lock is to allow people to do things out of the
> >no?
> >
> >It just helps to insure that it's less likely a programmer will
> >misuse the lock by declaring it outside of block scope (like at
> >class level). It doesn't fully enforce this, so it may be
> >Given the reasoning for it do you (and others) still think it
> >not throw in this case?
> Well there's nothing really wrong with having a lock as member data
> re-using it with a recursive mutex.

Yes, there most certainly is. The fact that you didn't catch this is
*precisely* why I want to discourage in every possible way this
misuse of the type. The documentation clearly states that this is a
misuse, and yet you think it's a reasonable thing to do. Let me
explain. Lock types are not thread safe. They maintain state to
indicate whether or not they've locked the associated mutex. This
state is not protected by synchronization objects in any way (and to
do so would be detrimental to use). A lock used recursively within a
class is being used for the "Active Object" pattern, in other words
to insure access to the object is internally thread safe. If the
lock were a class member, instead of properly created and destroyed
within each block needing synchronization, then it would become part
of the object's state and would need synchronization as well, lest
the class become not thread safe. See, using a lock outside of block
scope isn't just unusual usage, it's unsafe, and is clearly
documented as so.

> It's usually not the best way to
> lock, but I think that's true as soon as you use lock(). I think I
> change the lock classes so that they supported recursion, but
asserted if
> unlock() was called too many times.

The design was carefully thought out. I don't think that it's
> >> 9) I think the code would be clearer if member data names were
> >decorated
> >> somehow, eg fCount, mCount, _count, etc. :-)
> >
> >Stylistic suggestion, typically not entertained by Boost
> >submissions. However, I'm interested in this topic and wouldn't
> >hearing what others have to say on this, even if it's in private e-
> >mail.
> I know. :-) The code you posted is pretty clean, but it's rather
> disorienting to see a variable named "count" and have no clue as to
> whether it's member data, a local variable, or even a global.

I agree, and at one time it was decorated. Read my other post to see
why the decoration was removed. I'm open to using decorations here,
but I'm not willing to decide on a specific decoration myself and
doubt that Boost is going to declare one to be used.

Bill Kempf

Boost list run by bdawes at, gregod at, cpdaniel at, john at