Djikstra counting semaphore in c# for your review

  • Thread starter William Stacey [MVP]
  • Start date
T

TT \(Tom Tempelaere\)

William Stacey said:
Here is the link. If you find an issue or think of a feature, please post a
reply or send an email. Cheers!
http://www.mvptools.com/doco/csharp/semaphoredjikstra.htm

William,

About the Count property. I think this is a dangerous property to use. The
count is not guaranteed to be same when acquiring after checking the count.
It might be an optimization in some cases, but I don't see the use for it.

So perhaps then you shouldn't lock in the implementation of Count. What do
you think?

Cheers,
 
T

TT \(Tom Tempelaere\)

William Stacey said:
Here is the link. If you find an issue or think of a feature, please post a
reply or send an email. Cheers!
http://www.mvptools.com/doco/csharp/semaphoredjikstra.htm

William,

Your AcquireAll method tries to acquire each slot seperately. Perhaps you
could optimize and acquire all slots available each iteration until you
acquired all slots.

Plus the millisecondsTimeout is deceiving, because the max time could be
( maxSlots * millisecondsTimeout )
to acquire all slots.

I noticed that there exists a
Release( int n )
method but not an
Acquire( int n )
method. Is this the intention?

If you would write the Acquire( int n ) method, you could implement
Acquire() as Acquire( 1 )
and
AcquireAll() as Acquire( maxCount )

Cheers,
 
T

TT \(Tom Tempelaere\)

TT (Tom Tempelaere) said:
post

William,

About the Count property. I think this is a dangerous property to use. The
count is not guaranteed to be same when acquiring after checking the count.
It might be an optimization in some cases, but I don't see the use for it.

So perhaps then you shouldn't lock in the implementation of Count. What do
you think?

Perhaps it is necessary to introduce memory barriers so the count property
can't be optimized too heavily (ie inlining). Maybe making count volatile
would make the locking unnecessary...
 
W

William Stacey [MVP]

About the Count property. I think this is a dangerous property to use. The
count is not guaranteed to be same when acquiring after checking the count.
It might be an optimization in some cases, but I don't see the use for it.

Yes and no I think. I had thought the same thing while implementing.
However I look at like a hint, such as the count of a circular buffer. It
is only valid the instant you check it. Other threads could change this
directly after you "get" it. However the count is valid for that instant
and you never know how someone may want to use that. Maybe if only for perf
monitoring, or gui progress bar, etc.
 
W

William Stacey [MVP]

* The name is Dijkstra, not Djikstra (I'm dutch speaking, so I know
why)

Thanks Tom, you are correct. Thats for the update, I saw it both ways on
the INET. I will make the change. Cheers!
 
W

William Stacey [MVP]

Perhaps it is necessary to introduce memory barriers so the count property
can't be optimized too heavily (ie inlining). Maybe making count volatile
would make the locking unnecessary...

The lock and the Interlocked do introduce memory barriers. I don't see
current implementation as a performance issue. I still need the lock in the
other methods, so updating the count inside the lock seems like a good place
to leverage that lock. The Interlocked.CompareExchange only makes sure I
read the actual value atomically so it is like a mini-lock as it also has
mem barrier and deals with multi-processor cache issue. Volatile may also
work, not sure. I think I still want the increments and decrements to be
done inside the syncRoot lock, so also using volatile may just be overkill.
Other thoughts welcome.
 
W

William Stacey [MVP]

Your AcquireAll method tries to acquire each slot seperately. Perhaps you
could optimize and acquire all slots available each iteration until you
acquired all slots.

Probably. I just wanted to leverage existing method instead of writing more
duplicate logic.
Plus the millisecondsTimeout is deceiving, because the max time could be
( maxSlots * millisecondsTimeout )
to acquire all slots.

Yes, that is a side-effect. You could do some time calcs to get around the
right tot ms, but that starts to get messy - but is very doable.
I noticed that there exists a
Release( int n )
method but not an
Acquire( int n )
method. Is this the intention?

Yes. I looked at a lot of semaphore examples and none of them had
Acquire(int n). I guess the idea is one thread will only typically acquire
one slot. If you need more, you call twice, etc. The AcquireAll is just a
helper method if you need to acquireall for some reason to block others out
for awhile.

After looking at this again, however, I did discover an issue. The
AcquireAll locks the lock and loops inside. This would effectively block
other threads from doing a Release. I think removing the lock{} gives the
intended effect as the call to Acquire does its own locking (think this was
a carryover from another way I tried to implement that and did not remove
it.)

I will still think about the millisecond issue. This may be better handled
with documentation then updating the code? The intention is to provide some
safety valve instead of blocking forever.
 
T

TT \(Tom Tempelaere\)

TT (Tom Tempelaere) said:
William Stacey said:
Here is the link. If you find an issue or think of a feature, please
post
[...]
Plus the millisecondsTimeout is deceiving, because the max time could be
( maxSlots * millisecondsTimeout )
to acquire all slots.

I noticed that there exists a
Release( int n )
method but not an
Acquire( int n )
method. Is this the intention?

If you would write the Acquire( int n ) method, you could implement
Acquire() as Acquire( 1 )
and
AcquireAll() as Acquire( maxCount )

Here's my try to implement Acquire( int nrSlots ). Warning: it doesn't look
very efficient though :D.

public sealed class Semaphore
{
private int currentCount;
private readonly int maxCount;
private readonly object acquireLock = new object();
private readonly object starvationLock = new object();

public Semaphore( int currentCount, int maxCount )
{
Debug.Assert (
maxCount >= 1 &&
currentCount >= 0 &&
currentCount <= maxCount
);
this.currentCount = currentCount;
this.maxCount = maxCount;
}

public Semaphore( int maxCount )
: this( maxCount, maxCount )
{}

public bool Acquire()
{
return _Acquire_N( 1, Timeout.Infinite );
}

public bool Acquire( int timeoutMs )
{
return _Acquire_N( 1, timeoutMs );
}

public bool AcquireAll()
{
return _Acquire_N( maxCount, Timeout.Infinite );
}

public bool AcquireAll( int overallTimeoutMs )
{
return _Acquire_N( maxCount, overallTimeoutMs );
}

public bool Acquire_N( int requestedCount )
{
return _Acquire_N( requestedCount, Timeout.Infinite );
}

public bool Acquire_N( int requestedCount, int overallTimeoutMs )
{
return _Acquire_N( requestedCount, overallTimeoutMs );
}

private bool _Acquire_N( int requestedCount, int overallTimeoutMs )
{
Debug.Assert (
( requestedCount > 0 ) &&
( overallTimeoutMs > 0 || ( overallTimeoutMs == Timeout.Infinite ) )
);

IAcquireTimeoutTracker acqTimeoutTracker = ( overallTimeoutMs > 0 ) ?
(IAcquireTimeoutTracker) new RegularTimeoutTracker( overallTimeoutMs ) :
new InfiniteTimeoutTracker();
int acquiredCount = 0;

lock( acquireLock )
{
while( acquiredCount != requestedCount )
{
while( currentCount == 0 )
try
{
if( acqTimeoutTracker.HasElapsed ||
!Monitor.Wait( acquireLock, acqTimeoutTracker.RemainingWaitTime ) )
{
if( acquiredCount > 0 )
Release( acquiredCount );
return false;
}
}
catch
{
if( acquiredCount > 0 )
Release( acquiredCount );
Monitor.Pulse( acquireLock );
throw;
}

int targetCount = Math.Min( (requestedCount - acquiredCount),
currentCount );
acquiredCount += targetCount;
currentCount -= targetCount;

if( currentCount == 0 )
lock( starvationLock )
Monitor.PulseAll( starvationLock );
}
return true;
}
}

internal interface IAcquireTimeoutTracker
{
bool HasElapsed {
get;
}
int RemainingWaitTime {
get;
}
}
internal class InfiniteTimeoutTracker : IAcquireTimeoutTracker
{
public bool HasElapsed {
get {
return false;
}
}
public int RemainingWaitTime {
get {
return Timeout.Infinite;
}
}
}
internal class RegularTimeoutTracker : IAcquireTimeoutTracker
{
DateTime deadline;
TimeSpan timeout;
public RegularTimeoutTracker( int timeoutMs )
{
deadline = DateTime.Now + TimeSpan.FromMilliseconds( timeoutMs );
}
public bool HasElapsed {
get {
return (timeout = deadline - DateTime.Now) <= TimeSpan.Zero;
}
}
public int RemainingWaitTime {
get {
return timeout.Milliseconds;
}
}
}

public bool Release( int count )
{
// release implementation
return true;
}

// ...
}

Cheers,
 
T

TT \(Tom Tempelaere\)

"TT (Tom Tempelaere)" <_|\|_0§P@|/\|titi____AThotmail.com|/\|@P§0_|\|_>
wrote in message [...]
public sealed class Semaphore
{ [...]
private bool _Acquire_N( int requestedCount, int overallTimeoutMs )
{
Debug.Assert (
( requestedCount > 0 ) &&
( overallTimeoutMs > 0 || ( overallTimeoutMs == Timeout.Infinite ) )
);

IAcquireTimeoutTracker acqTimeoutTracker = ( overallTimeoutMs > 0 ) ?
(IAcquireTimeoutTracker) new RegularTimeoutTracker( overallTimeoutMs ) :
new InfiniteTimeoutTracker();
int acquiredCount = 0;

lock( acquireLock )
{
while( acquiredCount != requestedCount )

This would better be a do-while loop instead of a while loop of course
(albeit it just a small optimization).
{
while( currentCount == 0 )
try
{
if( acqTimeoutTracker.HasElapsed ||
!Monitor.Wait( acquireLock,
acqTimeoutTracker.RemainingWaitTime ) )
{
if( acquiredCount > 0 )
Release( acquiredCount );
return false;
}
}
catch
{
if( acquiredCount > 0 )
Release( acquiredCount );
Monitor.Pulse( acquireLock );
throw;
}

int targetCount = Math.Min( (requestedCount - acquiredCount),
currentCount );
acquiredCount += targetCount;
currentCount -= targetCount;

if( currentCount == 0 )
lock( starvationLock )
Monitor.PulseAll( starvationLock );
}
return true;
}
} [...]
}

Cheers,
 
W

William Stacey [MVP]

Very observant Tom. I can tell you are really studying the logic and
possible thread involved (as there are many in most locking primitives.)
However, this was another reason why I leveraged existing Acquire method.
If exception happens, the pulse is handled in Acquire and we just back-out
any slots we Acquired so far and throw the exception back to caller. The
release does not block so we should be able to release any slots we own
without an issue (except for slight lock issue below.)

However, you did remind me to bring up a point I wanted to make in the doco
and not sure if I did. It is possible to get an Interrupt or Abort
exception when *acquiring the monitor, even without a Wait(). A
Monitor.Enter() (which is what lock() starts with) could also throw
exception if the thread is Interrupted or Aborted directly before the
Enter(). I did not see any implementations that accounted for this slight
possibility either, and it would add more junk to the code, but I will have
to look over everything again with this specific issue in mind. Good
thoughts. Keep em coming. :) Cheers.
 
W

William Stacey [MVP]

Instead of more classes for timeouts, here is what I came up with (still
need to test.) I still need to look at your other code. Cheers!

public bool AcquireAll(int millisecondsTimeout)
{
int slotsGot = 0;
int elapsedMS = 0;
DateTime start = DateTime.Now;
int timeout = millisecondsTimeout;
for (int i = 0; i < maxCount; i++)
{
try
{
if (! Acquire(timeout) )
{
// Could not acquire all slots, release any we may already have got.
if ( slotsGot > 0 )
Release(slotsGot);
return false;
}
else
{
elapsedMS = (int)((TimeSpan)(DateTime.Now - start)).TotalMilliseconds;
timeout = millisecondsTimeout - elapsedMS; // Next wait will be a
smaller timeout.
if ( timeout < 0 )
timeout = 0; // Next Acquire will return false if we have to wait;
slotsGot++; // If we get all remaining slots with no timeout, we just
keep going.
}
}
catch
{
// Catch any exception during Acquire wait.
if ( slotsGot > 0 )
Release(slotsGot);
throw;
}
} // end for.
// Count is not zero, so notify any/all starvation consumers.
lock(starvationLock) { Monitor.PulseAll(starvationLock); }
return true;
}

--
William Stacey, MVP

TT (Tom Tempelaere) said:
message news:[email protected]...
William Stacey said:
Here is the link. If you find an issue or think of a feature, please
post
[...]
Plus the millisecondsTimeout is deceiving, because the max time could be
( maxSlots * millisecondsTimeout )
to acquire all slots.

I noticed that there exists a
Release( int n )
method but not an
Acquire( int n )
method. Is this the intention?

If you would write the Acquire( int n ) method, you could implement
Acquire() as Acquire( 1 )
and
AcquireAll() as Acquire( maxCount )

Here's my try to implement Acquire( int nrSlots ). Warning: it doesn't look
very efficient though :D.

public sealed class Semaphore
{
private int currentCount;
private readonly int maxCount;
private readonly object acquireLock = new object();
private readonly object starvationLock = new object();

public Semaphore( int currentCount, int maxCount )
{
Debug.Assert (
maxCount >= 1 &&
currentCount >= 0 &&
currentCount <= maxCount
);
this.currentCount = currentCount;
this.maxCount = maxCount;
}

public Semaphore( int maxCount )
: this( maxCount, maxCount )
{}

public bool Acquire()
{
return _Acquire_N( 1, Timeout.Infinite );
}

public bool Acquire( int timeoutMs )
{
return _Acquire_N( 1, timeoutMs );
}

public bool AcquireAll()
{
return _Acquire_N( maxCount, Timeout.Infinite );
}

public bool AcquireAll( int overallTimeoutMs )
{
return _Acquire_N( maxCount, overallTimeoutMs );
}

public bool Acquire_N( int requestedCount )
{
return _Acquire_N( requestedCount, Timeout.Infinite );
}

public bool Acquire_N( int requestedCount, int overallTimeoutMs )
{
return _Acquire_N( requestedCount, overallTimeoutMs );
}

private bool _Acquire_N( int requestedCount, int overallTimeoutMs )
{
Debug.Assert (
( requestedCount > 0 ) &&
( overallTimeoutMs > 0 || ( overallTimeoutMs == Timeout.Infinite ) )
);

IAcquireTimeoutTracker acqTimeoutTracker = ( overallTimeoutMs > 0 ) ?
(IAcquireTimeoutTracker) new RegularTimeoutTracker( overallTimeoutMs ) :
new InfiniteTimeoutTracker();
int acquiredCount = 0;

lock( acquireLock )
{
while( acquiredCount != requestedCount )
{
while( currentCount == 0 )
try
{
if( acqTimeoutTracker.HasElapsed ||
!Monitor.Wait( acquireLock,
acqTimeoutTracker.RemainingWaitTime ) )
 
T

TT \(Tom Tempelaere\)

TT (Tom Tempelaere) said:
[...]
Plus the millisecondsTimeout is deceiving, because the max time could be
( maxSlots * millisecondsTimeout )
to acquire all slots.
Here's my try to implement Acquire( int nrSlots ). Warning: it doesn't look
very efficient though :D.

Another issue is that lock could take some time and that should counted too
to check for timeout. Modifications are inside the code for _Acquire_N &
RegularTimeoutTracker.


public sealed class Semaphore
{
private int currentCount;
private readonly int maxCount;
private readonly object acquireLock = new object();
private readonly object starvationLock = new object();

public Semaphore( int currentCount, int maxCount )
{
Debug.Assert (
maxCount >= 1 &&
currentCount >= 0 &&
currentCount <= maxCount
);
this.currentCount = currentCount;
this.maxCount = maxCount;
}

public Semaphore( int maxCount )
: this( maxCount, maxCount )
{}

public bool Acquire()
{
return _Acquire_N( 1, Timeout.Infinite );
}

public bool Acquire( int timeoutMs )
{
return _Acquire_N( 1, timeoutMs );
}

public bool AcquireAll()
{
return _Acquire_N( maxCount, Timeout.Infinite );
}

public bool AcquireAll( int overallTimeoutMs )
{
return _Acquire_N( maxCount, overallTimeoutMs );
}

public bool Acquire_N( int requestedCount )
{
return _Acquire_N( requestedCount, Timeout.Infinite );
}

public bool Acquire_N( int requestedCount, int overallTimeoutMs )
{
return _Acquire_N( requestedCount, overallTimeoutMs );
}

private bool _Acquire_N( int requestedCount, int overallTimeoutMs )
{
Debug.Assert (
( requestedCount > 0 ) &&
( overallTimeoutMs > 0 || ( overallTimeoutMs == Timeout.Infinite ) )
);

IAcquireTimeoutTracker acqTimeoutTracker = ( overallTimeoutMs > 0 ) ?
(IAcquireTimeoutTracker) new RegularTimeoutTracker( overallTimeoutMs ) :
new InfiniteTimeoutTracker();
int acquiredCount = 0;

if( Monitor.TryEnter( acquireLock, acqTimeoutTracker.RemainingWaitTime ) )
try
{
do
{
while( currentCount == 0 )
if( acqTimeoutTracker.HasElapsed ||
!Monitor.Wait( acquireLock, acqTimeoutTracker.RemainingWaitTime ) )
throw new TimeoutException();

int targetCount = Math.Min( (requestedCount - acquiredCount),
currentCount );
acquiredCount += targetCount;
currentCount -= targetCount;

if( currentCount == 0 )
lock( starvationLock )
Monitor.PulseAll( starvationLock );
}
while( acquiredCount != requestedCount );
return true;
}
catch( Exception e )
{
if( acquiredCount > 0 )
Release( acquiredCount );
Monitor.Pulse( acquireLock );
if( !(e is TimeoutException) )
throw;
}
finally
{
Monitor.Exit( acquireLock );
}

return false;
}

private class TimeoutException : Exception
{
public override string Message {
get {
return "Waiting timeout";
}
}
}

internal interface IAcquireTimeoutTracker
{
bool HasElapsed {
get;
}
int RemainingWaitTime {
get;
}
}
internal class InfiniteTimeoutTracker : IAcquireTimeoutTracker
{
public bool HasElapsed {
get {
return false;
}
}
public int RemainingWaitTime {
get {
return Timeout.Infinite;
}
}
}
internal class RegularTimeoutTracker : IAcquireTimeoutTracker
{
DateTime deadline;
TimeSpan timeout;
public RegularTimeoutTracker( int timeoutMs )
{
this.deadline = DateTime.Now + TimeSpan.FromMilliseconds( timeoutMs );
this.timeout = deadline - DateTime.Now;
}
public bool HasElapsed {
get {
return (timeout = deadline - DateTime.Now) <= TimeSpan.Zero;
}
}
public int RemainingWaitTime {
get {
Debug.Assert (
timeout.Milliseconds > 0
);
return timeout.Milliseconds;
}
}
}

public bool Release( int count )
{
return true;
}
}


Looks better. Only starvation isn't checked for timeout.

Tom T.
 
T

TT \(Tom Tempelaere\)

William Stacey said:
Here is the link. If you find an issue or think of a feature, please post a
reply or send an email. Cheers!
http://www.mvptools.com/doco/csharp/semaphoredjikstra.htm

I think you should forbid infinite timeouts, because in some patterns there
may be deadlocks. For instance if 2 clients (read: threads) of the semaphore
execute AcquireAll, and another (third) client already has one or a few
slots acquired. Thread 1 gets half the remainig slots and Thread2 gets the
other half. Then the third client releases but it wont have any effect.
Thread 1 & Thread 2 will never release their half so there is a deadlock.

I don't know if you should take that into account but it is possible.
 
W

William Stacey [MVP]

What you say is true. However this patten is used by other lock primitives
like Monitor, where it is also possible to dead-lock yourself on infinite
timeout. As this is meant to be general and flexible (like Monitor), this
is an issue for the user to decide and program around - hence the timeout
overloads. If you have one producer and one consumer (binary sem for
example), this may be the exact behavior you want. You may want to block
forever until something is in a queue for example. Pooling on a timeout
really just brings you back to the same issue - what do you do if you
timeout? In most cases, you just wait again as you never know when you will
get signaled. If you truly want to wait only N ms, then you have that
option. This is always one of the challenges with threading, deciding how
long is long enouph for timeouts and then what to do when you do timeout? I
see your point, however there is not really a good solution to this imo -
just something each program will need to decide for itself. Naturally, the
control program can also Interrupt the threads, which is handled here
correctly I think. Cheers!
 
W

William Stacey [MVP]

I suppose one way to head off this specific issue is with another lock (i.e.
acquireAllLock) in the AcquireAll method. Allow only one thread at a time
to enter passed the acquireAllLock exclusive lock. This gets you a little
further, but is not a complete solution as the first AcquireAll() could
still block forever waiting on a Release() that never comes as the owner is
blocked for some reason. At some point, there is only so much you can do.
You can provide a tool, but you can't prevent the user from hitting
themselfs in the head with it when your not looking. Cheers.

--
William Stacey, MVP

William Stacey said:
What you say is true. However this patten is used by other lock primitives
like Monitor, where it is also possible to dead-lock yourself on infinite
timeout. As this is meant to be general and flexible (like Monitor), this
is an issue for the user to decide and program around - hence the timeout
overloads. If you have one producer and one consumer (binary sem for
example), this may be the exact behavior you want. You may want to block
forever until something is in a queue for example. Pooling on a timeout
really just brings you back to the same issue - what do you do if you
timeout? In most cases, you just wait again as you never know when you will
get signaled. If you truly want to wait only N ms, then you have that
option. This is always one of the challenges with threading, deciding how
long is long enouph for timeouts and then what to do when you do timeout? I
see your point, however there is not really a good solution to this imo -
just something each program will need to decide for itself. Naturally, the
control program can also Interrupt the threads, which is handled here
correctly I think. Cheers!
 
T

TT \(Tom Tempelaere\)

William Stacey said:
I suppose one way to head off this specific issue is with another lock (i.e.
acquireAllLock) in the AcquireAll method. Allow only one thread at a time
to enter passed the acquireAllLock exclusive lock. This gets you a little
further, but is not a complete solution as the first AcquireAll() could
still block forever waiting on a Release() that never comes as the owner is
blocked for some reason. At some point, there is only so much you can do.
You can provide a tool, but you can't prevent the user from hitting
themselfs in the head with it when your not looking. Cheers.

I think that it should indeed be the responsability of the user of the
semaphore object to avoid such situations.

Tom T.
 
T

TT \(Tom Tempelaere\)

William Stacey said:
I suppose one way to head off this specific issue is with another lock (i.e.
acquireAllLock) in the AcquireAll method. Allow only one thread at a time
to enter passed the acquireAllLock exclusive lock. This gets you a little
further, but is not a complete solution as the first AcquireAll() could
still block forever waiting on a Release() that never comes as the owner is
blocked for some reason. At some point, there is only so much you can do.
You can provide a tool, but you can't prevent the user from hitting
themselfs in the head with it when your not looking. Cheers.

I think that it should indeed be the responsability of the user of the
semaphore object to avoid such situations.

Tom T.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Swiftpoint Z Mouse 2
Suggestions for the future of PC Review 50
Practicla .NET2 and C#2 7
Improving PC Review 15
Samsung NX200 Camera 0
peer to peer in C# .NET 2
Ask not 8
How to detect the real status of a socket? 4

Top