Really (!) correct Semaphore

Discussion in 'Java' started by Frank Gerlach, Nov 27, 2004.

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

    /** Semaphore class for managing limited resources (such as database
    * connections)
    * Author: Frank Gerlach ()
    * 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()
    }
    }
     
    Frank Gerlach, Nov 27, 2004
    #1
    1. Advertising

  2. Frank Gerlach

    thirdrock Guest

    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
     
    thirdrock, Nov 27, 2004
    #2
    1. Advertising

  3. Frank Gerlach

    Chris Smith Guest

    Frank Gerlach <> wrote:
    > 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
     
    Chris Smith, Nov 27, 2004
    #3
  4. Frank Gerlach

    Chris Smith Guest

    thirdrock <> wrote:
    > > 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.


    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
     
    Chris Smith, Nov 27, 2004
    #4
  5. Frank Gerlach

    thirdrock Guest

    Chris Smith wrote:
    > thirdrock <> wrote:
    >
    >>> 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.

    >
    >
    > 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
     
    thirdrock, Nov 28, 2004
    #5
  6. Chris Smith <> wrote in message news:<>...
    > Frank Gerlach <> wrote:
    > > 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.

    OK.
    >
    > > public Semaphore(int initialcount){
    > > count=initialcount;
    > > }

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

    OK.
     
    Frank Gerlach, Nov 28, 2004
    #6
  7. Frank Gerlach

    Chris Smith Guest

    Frank Gerlach <> wrote:
    > > 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().


    Okay, fine.

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

    Chris Smith - Lead Software Developer/Technical Trainer
    MindIQ Corporation
     
    Chris Smith, Nov 28, 2004
    #7
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. dede
    Replies:
    0
    Views:
    604
  2. Dima
    Replies:
    0
    Views:
    2,255
  3. Frank Gerlach
    Replies:
    34
    Views:
    15,371
  4. Victor Bazarov

    semaphore

    Victor Bazarov, Jul 4, 2003, in forum: C++
    Replies:
    3
    Views:
    881
    chiew peng
    Jul 4, 2003
  5. techi_C
    Replies:
    2
    Views:
    1,453
    Richard Bos
    Aug 10, 2006
Loading...

Share This Page