Re: Boost.Threads draft library submission

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 
the 
> >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 
or
> >> INVALID_HANDLE_VALUE.
> >
> >Other's have pointed out the lack of error handling.  I did this 
> >intentionally in the draft, but it's something that needs 
addressed 
> >before submission.
> 
> I didn't see mention of the error handling on the Windows side, but 
it 
> 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 
if 
> >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 
in 
> >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 
doesn't
> >> 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 
an 
> 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 
ReleaseSemaphore 
> documentation in MSDN doesn't mention what error codes are returned 
so 
> you might have to go by the error your system is returning which 
doesn't 
> 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 
exceptional 
> 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 
error.  
> >An assert is not enough.
> 
> IMO returning an error code is the worst of the three alternatives. 
An 
> assert arguably is enough. The only times it wouldn't be enough is 
for 
> those developers foolish enough to run without assert enabled or 
that 
> code path was never tested before the software was released. Either 
of 
> 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 
any 
> >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 
ordinary, 
> >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 
the 
> >class level).  It doesn't fully enforce this, so it may be 
overkill.  
> >Given the reasoning for it do you (and others) still think it 
should 
> >not throw in this case?
> 
> Well there's nothing really wrong with having a lock as member data 
and 
> 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 
would 
> 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 
incorrect.
 
> >> 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 
mind 
> >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