Re: [boost] Re: Boost.Threads draft library submission

From: Jesse Jones (jesjones_at_[hidden])
Date: 2001-03-15 20:25:14

>--- 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.

>> 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.

>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.

>> 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".

> 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.

>> 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. 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.

>> 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.

  -- Jesse