Really (!) correct Semaphore

F

Frank Gerlach

I hope I got it right this time. Please comment.

/** Semaphore class for managing limited resources (such as database
* connections)
* Author: Frank Gerlach ([email protected])
* Copyright: None. Use in any way you want.
*/
public class Semaphore{
int count;
/** Create the semaphore
* @param initialcount number of resource items to manage
*/
public Semaphore(int initialcount){
count=initialcount;
}

/** Acquire one resource item.
*/
public synchronized void P() throws InterruptedException{
while(count==0){
wait();
}
if(count<0)throw new Error("Fatal problem in Semaphore");//just in case
count--;
}

/** Release one resource item.
*/
public synchronized void V(){
count++;
notify();//always notify(), necessary if multiple threads sleep in P()
}
}
 
T

thirdrock

Frank Gerlach wrote:
Potentially a couple of problems.
public class Semaphore{
You probably want to make 'count' private.
int count;
/** Create the semaphore
* @param initialcount number of resource items to manage
*/
public Semaphore(int initialcount){
count=initialcount;
}

/** Acquire one resource item.
*/
public synchronized void P() throws InterruptedException{
while(count==0){
wait();

Are you going to wait forever here? It's usually good practice to not
have any place in your code that can be caught in an infinite loop. You
might want to limit the time you wait, then throw an exception.
}
if(count<0)throw new Error("Fatal problem in Semaphore");//just in case
You probably want to use a critical section when you change 'count' so
as there is no contention.
count--;
}

/** Release one resource item.
*/
public synchronized void V(){
As above, you should use a critical section to make changes to 'count'.
count++;
notify();//always notify(), necessary if multiple threads sleep in P()
}
}

Enjoy,

Ian
 
C

Chris Smith

Frank Gerlach said:
I hope I got it right this time. Please comment.

You've pretty much got it right. I'd change three things, if you're
asking me to be picky:
public class Semaphore{
int count;

Make that private.
public Semaphore(int initialcount){
count=initialcount;
}

Perform a simple range check here:

if (count <= 0) throw new IllegalArgumentException(initialcount);
while(count==0){
wait();
}
if(count<0)throw new Error("Fatal problem in Semaphore");//just in case

That's dead code, and I wouldn't put it in as a matter of style. It
reeks of trial-and-error programming, and communicates to the reader
that you aren't really certain that you've got the synchronization
right. Be confident: your synchronization is fine.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation
 
C

Chris Smith

thirdrock said:
Are you going to wait forever here? It's usually good practice to not
have any place in your code that can be caught in an infinite loop.

Nonsense. You can't preempt every possible poor usage of an API, and
simplicity is its own goal. If you need a semaphore that can time out,
then Frank's implementation isn't for you; but 99% of semaphores don't
need to time out, and I'd be really peeved if Frank guessed about how
long a P() might need to wait and was wrong, and it crashed my
production application as a result.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation
 
T

thirdrock

Chris said:
Nonsense. You can't preempt every possible poor usage of an API, and
simplicity is its own goal. If you need a semaphore that can time out,
then Frank's implementation isn't for you; but 99% of semaphores don't
need to time out,
Hmmm, I seem to recall from another API (not java) a function called
WaitForObject(HANDLE object, int milliseconds). If milliseconds == 0
then it waits forever, otherwise, it waits for as long as you are
prepared to wait before doing something else.
If this code was being executed from a servlet, and the http timeout was
60 seconds, would you want the semaphore waiting longer than 60 seconds?
and I'd be really peeved if Frank guessed about how
long a P() might need to wait and was wrong,

Yes, I would be peeved if I couldn't pass a parameter with the maximum
time I was prepared to wait for the semaphore.
and it crashed my
production application as a result.

I have no idea how a SemaphoreTimedOut exception could crash your
production application. I have to assume that you don't catch exceptions
in your production applications.

Ian
 
F

Frank Gerlach

Chris Smith said:
You've pretty much got it right. I'd change three things, if you're
asking me to be picky:


Make that private. OK.

Perform a simple range check here:

if (count <= 0) throw new IllegalArgumentException(initialcount);
(count<0) would be the right thing, I assume. There could be a situation when
initially no resources are available and some kind of producers would have to
do their work and then call V().
That's dead code, and I wouldn't put it in as a matter of style. It
reeks of trial-and-error programming, and communicates to the reader
that you aren't really certain that you've got the synchronization
right. Be confident: your synchronization is fine.
OK.
 
C

Chris Smith

Frank Gerlach said:
(count<0) would be the right thing, I assume. There could be a situation when
initially no resources are available and some kind of producers would have to
do their work and then call V().

Okay, fine.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation
 

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

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top