William Stacey said:
Update to the code? I did post a few changes after the first post. Let me
know what updates you would like to see.
I forgot to check the link before posting, I see that you already made some
changes. Woops.
Have you already done some tests under stress? A working project that uses
the semaphore to demonstrate that it works ...
The code-documentation in AcquireAll, just before the lock on starvationLock
is a typo: it sais "... is not zero ..." but I think it should read "... is
now zero ...".
I think you should introduce Debug.Assert's at specific places for several
reasons, which is my opinion and perhaps a question of style (or QOI).
* clients of your library made a mistake in using your class/lib
* you made a mistake (aka bug) when making your class/lib
* when you refactor these can prove invaluable (I say this from experience)
* to highlight some pre-conditions or post-conditions. It looks like proof
then, and they will probably never even trigger. It helps the reviewer of
the library and yourself when you return back to the project after a long
time.
* in some libraries (eg in-house) you would not throw exceptions when
receiving bad parameters (or a bad combination), just assert. When debugging
the developer can use the code as a tool to know what he did wrong in using
the library. He should correct things if the asserts trigger and afterwards
the assert will no longer trigger. In that case the checking + throwing code
is no longer necessary and becomes code bloat. On the other hand in some
circumstances you would prefer to throw (for stability reasons), it all
depends on the type of library and who you are writing it for. MFC library
is a good example, it has a lot of asserts. A quick count of "ASSERT" (match
case, match whole word) in code of vc6 mfc library gave 5618 occurences.
Just to mention.
* Asserts could reveal nasty sneaky threading issues (because code execution
isn't linear anymore). Throwing pulls you out of the context in which the
state-error occured.
I'm not saying that this is all applicable to your class of course, but it
is something that I think is very valuable and it is something that I like
to see in library-like classes.
Another thing is that maxCount should probably be strictly greater than one.
A Mutex (or just lock) is a .Net primitive which is probably somewhat more
efficient and closer to the OS. On the other hand, you would not have the
starvation trigger though. Maybe a small docu-entry to the class ;-)?
Because reverse-sensing semaphore with maxSlots=1 will probably have its
use.
I don't understand the use for TryRelease. The implementation of Release is
almost the same, the difference is that you throw in Release and return true
or false in TryRelease. Why would someone try to release and not just
release? The way I see it, if releaseCount<=0 or if (count + releaseCount) >
maxCount then that's an error of the user of semaphore. I think you don't
need two methods that do the same thing but indicate errors in a different
way.
I think I'd prefer a TryAcquire (which is nothing more then "return
Acquire(0);") or TryAcquireAll (which is not the same as AcquireAll(0) since
all slots are acquired individually). They acquire if a slot is free (or all
slots are free) and return true if they did, otherwise they don't acquire
anything and return false.
ReleaseAll is a dangerous function now. I don't know if I would allow it to
be called if count isn't zero. That is something to think about.
In your implementation of Acquire there is a (very) small chance of
inconsistency in the face of a Thread.Interrupt call and the exception that
is a result of this. I've added some code to counter this (check below). The
same issue is present in AcquireAll.
Explanation. The exception could occur when you lock (Mutex under the hood).
If the exception occurs, the caller will think that the slot was not
acquired since the exception percolated through, but you already decremented
count when it happens.
public bool Acquire( int millisecondsTimeout )
{
lock( syncLock )
{
while( count == 0 )
try
{
if( !Monitor.Wait( syncLock, millisecondsTimeout ) )
return false;
}
catch
{
Monitor.Pulse(syncLock);
throw;
}
if( --count == 0 )
try
{
lock( starvationLock )
Monitor.PulseAll( starvationLock );
}
catch // guard against ThreadInterruptedException
{
++count;
throw;
}
return true;
}
}
I am very cautious with this because I tend to use Thread.Interrupt to wake
threads. Thread.Abort throws too but there is no good way to protect against
that of course. Anyway, ...
Cheers,
