Simple ScheduledFuture problem

classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Simple ScheduledFuture problem

iksrazal
Hi all,

I've seen similair questions on the list but I'm still stuck. I just
want to timeout a future that is scheduled at a fixed rate. The code
below works, but does not run at a scheduled rate for about two
minutes until it recovers after timeout! Maybe I should pass a
ThreadFactory to the Executor?  I'm unclear how to apply
future.get(timeout) when using a fixed rate. Please help, code below
(just a quick hack, nothing serious) . What times out is
URLConnectiion.openConnection();

package org;

import java.io.BufferedReader;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.InetAddress;
import java.net.Socket;
import java.net.URL;
import java.net.URLConnection;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.Timer;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

public class Ping {
       
          private int flag = 0;
          private static int STOPPED = new Integer(0);
          private static int BEGIN_INIT = new Integer(1);
          private static int RUNNING = new Integer(2);
          private static Map <Integer, String> map;
       
          /** commons logging declaration. */
          private static Log logger = LogFactory.getLog(
            Ping.class);

          /** Timer expressed in milliseconds as six seconds */
          private final static long ONCE_PER_SIX_SECONDS = 1000*6;

          public static void main(String[] args) throws Exception {

              new Ping();
          }

          Ping () {
              boolean interrupted = false;
              ScheduledExecutorService ses =
Executors.newSingleThreadScheduledExecutor();
              // Do pings, starting now, with a 6 second delay
              ScheduledFuture <?> ping = ses.scheduleAtFixedRate(new
PingTask(),
                  0L, 6000L, TimeUnit.MILLISECONDS);
          }

          static {
              map = new HashMap<Integer, String>();
              map.put(STOPPED, "STOPPED");
              map.put(BEGIN_INIT, "BEGIN_INIT");
              map.put(RUNNING, "RUNNING");
          }

          class PingTask implements Runnable {

            private void doConnect(String host, int port, boolean
on_connect, int state) {
                try {
                    InetAddress addr = InetAddress.getByName(host);
                    // will throw an exception if could not connect
                    Socket s = new Socket(addr, port);
                    s.close();
                    if (on_connect) {
                        logger.debug("Found port: " + port);
                        setFlag(state);
                    }

                } catch (Exception ex) {
                    logger.error("Can't find port: " + port);
                    logger.error(ex.getMessage(), ex);
                    if (!on_connect) {
                        setFlag(state);
                    }
                }

            }

            private void doConnect(URL host, boolean on_connect, int state) {
                try {
                    logger.debug("connecting to url: " + host.toString());
                    URLConnection uc = host.openConnection();
                    if (uc == null) {
                        logger.error("Got a null URLConnection object!");
                        return;
                    }
                    InputStream is = uc.getInputStream();
                    if (is == null) {
                        logger.error("Got a null content object!");
                        return;
                    }
                    BufferedReader in = new BufferedReader(
                        new InputStreamReader(
                        is));
                    String inputLine;
                    // just test that its readable for now
                    while ((inputLine = in.readLine()) != null)  {
                        ;
                    }
                    in.close();
                    if (on_connect) {
                        logger.debug("Found url: " + host.toString());
                        setFlag(state);
                    }

                } catch (Exception ex) {
                    logger.error("Can't find url: " + host.toString());
                    logger.error(ex.getMessage(), ex);
                    if (!on_connect) {
                        setFlag(state);
                    }
                }

          }


          public void run() {

              Timer timer = new Timer(true);
              try {
                  // Schedule a 5 second interupt for the timeout - ping
should complete
                  // in less than 5 seconds
                  timer.schedule(new TimeOutTask(Thread.currentThread()), 5000);
                  if (flag == STOPPED) {
                      doConnect("localhost", 1099, true, BEGIN_INIT);
                  }
                  if (flag == BEGIN_INIT) {
                      // test state of stopped to prevent endless loop
                      doConnect("localhost", 1099, false, STOPPED);
                      doConnect(new
URL("http://localhost:8080/maragato/"), true, RUNNING);
                  }
                  if (flag == RUNNING) {
                      doConnect("localhost", 1099, false, STOPPED);
                  }
                } catch (Exception ex) {
                    logger.error(ex.getMessage(), ex);
                }
                finally {
                    logger.debug("CURRENT STATE: " + getMessage(flag));
                    // task must be cancelled so Thread.interrupt() is
_not_ called
                    timer.cancel();
                }
            }
        }

        private synchronized void setFlag(int var) {
            flag = var;
        }

        /** Get message via an int.
         * @param code Integer mapped to message
         * @return String mapped message
         */
        public final String getMessage(int code) {
            return map.get(code);
        }
}

package org;

import java.util.TimerTask;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

public class TimeOutTask extends TimerTask {
       
         /** commons logging declaration. */
    private static Log logger = LogFactory.getLog(
    TimeOutTask.class);
       
        Thread t;

        TimeOutTask(Thread t) {
            this.t = t;
        }

        /**
        An implementation of the Abstract Class TimerTask method run()
        */
        public void run() {
            if(t!= null && t.isAlive()) {
                t.interrupt();
                logger.error("thread: " + t.getName() + ", task timed out!");
            }
            else {
            logger.error("task timed out, but could not interrupt!");
            }
        }
}
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

David Holmes-3
Robert,

I'm not sure what the delay is in restarting the scheduled task, but
regarding your query on using future.get(timeout) with a fixed-rate task, my
understanding is that get() will return the last computed result (if there
is one) until the next computation cycle begins. In other words if the task
is between executions then get() returns the result from the last execution,
otherwise it blocks until the current execution completes.

But I'm not sure what thread would be calling it in your example. It seems
you want the task to be unblocked not some thread waiting on the output of
the task.

Cheers,
David Holmes

> -----Original Message-----
> From: [hidden email]
> [mailto:[hidden email]]On Behalf Of robert
> lazarski
> Sent: Thursday, 17 August 2006 7:34 AM
> To: [hidden email]
> Subject: [concurrency-interest] Simple ScheduledFuture problem
>
>
> Hi all,
>
> I've seen similair questions on the list but I'm still stuck. I just
> want to timeout a future that is scheduled at a fixed rate. The code
> below works, but does not run at a scheduled rate for about two
> minutes until it recovers after timeout! Maybe I should pass a
> ThreadFactory to the Executor?  I'm unclear how to apply
> future.get(timeout) when using a fixed rate. Please help, code below
> (just a quick hack, nothing serious) . What times out is
> URLConnectiion.openConnection();
>
> package org;
>
> import java.io.BufferedReader;
> import java.io.InputStream;
> import java.io.InputStreamReader;
> import java.net.InetAddress;
> import java.net.Socket;
> import java.net.URL;
> import java.net.URLConnection;
> import java.util.HashMap;
> import java.util.Map;
> import java.util.concurrent.Executors;
> import java.util.concurrent.ScheduledExecutorService;
> import java.util.concurrent.ScheduledFuture;
> import java.util.concurrent.TimeUnit;
> import java.util.Timer;
>
> import org.apache.commons.logging.Log;
> import org.apache.commons.logging.LogFactory;
>
> public class Ping {
>
>  private int flag = 0;
>  private static int STOPPED = new Integer(0);
>  private static int BEGIN_INIT = new Integer(1);
>  private static int RUNNING = new Integer(2);
>           private static Map <Integer, String> map;
>
>  /** commons logging declaration. */
>  private static Log logger = LogFactory.getLog(
>     Ping.class);
>
>  /** Timer expressed in milliseconds as six seconds */
>  private final static long ONCE_PER_SIX_SECONDS = 1000*6;
>
>  public static void main(String[] args) throws Exception {
>
>               new Ping();
>  }
>
>           Ping () {
>               boolean interrupted = false;
>               ScheduledExecutorService ses =
> Executors.newSingleThreadScheduledExecutor();
>               // Do pings, starting now, with a 6 second delay
>               ScheduledFuture <?> ping = ses.scheduleAtFixedRate(new
> PingTask(),
>                   0L, 6000L, TimeUnit.MILLISECONDS);
>  }
>
>           static {
>               map = new HashMap<Integer, String>();
>               map.put(STOPPED, "STOPPED");
>               map.put(BEGIN_INIT, "BEGIN_INIT");
>               map.put(RUNNING, "RUNNING");
>           }
>
>           class PingTask implements Runnable {
>
>             private void doConnect(String host, int port, boolean
> on_connect, int state) {
>                 try {
>                     InetAddress addr = InetAddress.getByName(host);
>                     // will throw an exception if could not connect
>                     Socket s = new Socket(addr, port);
>                     s.close();
>                     if (on_connect) {
>                         logger.debug("Found port: " + port);
>                         setFlag(state);
>                     }
>
>                 } catch (Exception ex) {
>                     logger.error("Can't find port: " + port);
>                     logger.error(ex.getMessage(), ex);
>                     if (!on_connect) {
>                         setFlag(state);
>                     }
>                 }
>
>             }
>
>             private void doConnect(URL host, boolean on_connect,
> int state) {
>                 try {
>                     logger.debug("connecting to url: " + host.toString());
>                     URLConnection uc = host.openConnection();
>                     if (uc == null) {
>                         logger.error("Got a null URLConnection object!");
>                         return;
>                     }
>                     InputStream is = uc.getInputStream();
>                     if (is == null) {
>                         logger.error("Got a null content object!");
>                         return;
>                     }
>                     BufferedReader in = new BufferedReader(
>                         new InputStreamReader(
>                         is));
>                     String inputLine;
>                     // just test that its readable for now
>                     while ((inputLine = in.readLine()) != null)  {
>                         ;
>                     }
>                     in.close();
>                     if (on_connect) {
>                         logger.debug("Found url: " + host.toString());
>                         setFlag(state);
>                     }
>
>                 } catch (Exception ex) {
>                     logger.error("Can't find url: " + host.toString());
>                     logger.error(ex.getMessage(), ex);
>                     if (!on_connect) {
>                         setFlag(state);
>                     }
>                 }
>
>           }
>
>
>           public void run() {
>
>      Timer timer = new Timer(true);
>               try {
>          // Schedule a 5 second interupt for the timeout - ping
> should complete
>          // in less than 5 seconds
>          timer.schedule(new
> TimeOutTask(Thread.currentThread()), 5000);
>                   if (flag == STOPPED) {
>                       doConnect("localhost", 1099, true, BEGIN_INIT);
>                   }
>                   if (flag == BEGIN_INIT) {
>                       // test state of stopped to prevent endless loop
>                       doConnect("localhost", 1099, false, STOPPED);
>                       doConnect(new
> URL("http://localhost:8080/maragato/"), true, RUNNING);
>                   }
>                   if (flag == RUNNING) {
>                       doConnect("localhost", 1099, false, STOPPED);
>                   }
>                 } catch (Exception ex) {
>                     logger.error(ex.getMessage(), ex);
>                 }
>                 finally {
>                     logger.debug("CURRENT STATE: " + getMessage(flag));
>                     // task must be cancelled so Thread.interrupt() is
> _not_ called
>            timer.cancel();
>                 }
>             }
>         }
>
>         private synchronized void setFlag(int var) {
>             flag = var;
>         }
>
>         /** Get message via an int.
>          * @param code Integer mapped to message
>          * @return String mapped message
>          */
>         public final String getMessage(int code) {
>             return map.get(code);
>         }
> }
>
> package org;
>
> import java.util.TimerTask;
>
> import org.apache.commons.logging.Log;
> import org.apache.commons.logging.LogFactory;
>
> public class TimeOutTask extends TimerTask {
>
> /** commons logging declaration. */
>     private static Log logger = LogFactory.getLog(
>     TimeOutTask.class);
>
> Thread t;
>
> TimeOutTask(Thread t) {
>    this.t = t;
> }
>
> /**
> An implementation of the Abstract Class TimerTask method run()
> */
> public void run() {
>    if(t!= null && t.isAlive()) {
>        t.interrupt();
>        logger.error("thread: " + t.getName() + ", task
> timed out!");
>    }
>    else {
>     logger.error("task timed out, but could not interrupt!");
>    }
> }
> }
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

Joe Bowbeer
In reply to this post by iksrazal
At a glance, I notice a few things that could be cleaned up, though
they may have no impact on the problem.

1. setFlag is synchronized but there is no synchronized getFlag method.

Either added synchronized getFlag, or declare flag to be "volatile".

2. doConnect(url) code may fail without closing input stream.

Add try-catch after opening input stream.

3. Also, I would consider switching to fixed delay to avoid the
possibility of multiple outstanding pings.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

iksrazal
Thanks Dave and Joe. I implemented the changes you two mentioned (I
hope) and now invoke a FutureTask inside the Runnable of the
ScheduledFuture . Here's what I came up with. The purpose here is just
to simply record the states of an app server.

Thanks!
Robert

package org;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.InetAddress;
import java.net.Socket;
import java.net.URL;
import java.net.URLConnection;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.Executors;
import java.util.concurrent.FutureTask;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

public class Ping {

      private volatile int  flag = 0;
      private static int STOPPED = new Integer(0);
      private static int BEGIN_INIT = new Integer(1);
      private static int RUNNING = new Integer(2);
          private static Map <Integer, String> map;

      /** commons logging declaration. */
      private static Log logger = LogFactory.getLog(
                Ping.class);

      public static void main(String[] args) throws Exception {

              new Ping();
      }

      Ping () {
          ScheduledExecutorService ses =
Executors.newSingleThreadScheduledExecutor();
          // Do pings, starting now, with a 2 second delay in between
          ScheduledFuture <?> ping = ses.scheduleWithFixedDelay(new PingTask(),
              0L, 2000L, TimeUnit.MILLISECONDS);
      }

      static {
          map = new HashMap<Integer, String>();
          map.put(STOPPED, "STOPPED");
          map.put(BEGIN_INIT, "BEGIN_INIT");
          map.put(RUNNING, "RUNNING");
      }

      private synchronized void setFlag(int var) {
          flag = var;
      }

      /** Get message via an int.
       * @param code Integer mapped to message
       * @return String mapped message
       */
      public final String getMessage(int code) {
          return map.get(code);
      }

      class PingFuture implements Runnable {
          private void doConnect(String host, int port, boolean
on_connect, int state) {
              try {
                  logger.debug("connecting to host: " +host+ ", port: " + port);
                  InetAddress addr = InetAddress.getByName(host);
                  // will throw an exception if could not connect
                  Socket s = new Socket(addr, port);
                  s.close();
                  if (on_connect) {
                      logger.debug("Found port: " + port);
                      setFlag(state);
                  }

              } catch (Exception ex) {
                  logger.error("Can't find port: " + port);
                  logger.error(ex.getMessage(), ex);
                  if (!on_connect) {
                      setFlag(state);
                  }
              }

          }

          private void doConnect(URL host, boolean on_connect, int
state) throws IOException {
              InputStream is = null;
              BufferedReader in = null;
              try {
                  logger.debug("connecting to url: " + host.toString());
                  URLConnection uc = host.openConnection();
                  if (uc == null) {
                      logger.error("Got a null URLConnection object!");
                      return;
                  }
                  is = uc.getInputStream();
                  if (is == null) {
                      logger.error("Got a null content object!");
                      return;
                  }
                  in = new BufferedReader(new InputStreamReader(
                        is));
                  String inputLine;
                  // just test that its readable for now
                  while ((inputLine = in.readLine()) != null)  {
                        ;
                  }
                  if (on_connect) {
                      logger.debug("Found url: " + host.toString());
                      setFlag(state);
                  }

              } catch (Exception ex) {
                  logger.error("Can't find url: " + host.toString());
                  logger.error(ex.getMessage(), ex);
                  if (!on_connect) {
                      setFlag(state);
                  }
              } finally {
                  if (is != null) {
                      is.close();
                  }
                  if (in != null) {
                      in.close();
                  }
              }

          }

          public void run() {

              try {
                  if (flag == STOPPED) {
                      doConnect("localhost", 1099, true, BEGIN_INIT);
                  }
                  if (flag == BEGIN_INIT) {
                      // test state of stopped to prevent endless loop
                      doConnect("localhost", 1099, false, STOPPED);
                      doConnect(new
URL("http://localhost:8080/maragato/"), true, RUNNING);

                  }
                  if (flag == RUNNING) {
                      doConnect(new
URL("http://localhost:8080/maragato/"), false, STOPPED);
                  }

              } catch (Exception ex) {
                    logger.error(ex.getMessage(), ex);
              }
          }
      } // end inner class PingFuture

      class PingTask implements Runnable {

          public void run() {

              try {
                  FutureTask<?> f = new FutureTask<Object>(new
PingFuture(), null);
                  Thread thread = new Thread(f);
                  thread.start();
                  // 5 seconds to finish connect or will timeout
                  f.get(5000, TimeUnit.MILLISECONDS);
              } catch (Exception ex) {
                    logger.error(ex.getMessage(), ex);
              }
              finally {
                  logger.debug("CURRENT STATE: " + getMessage(flag));

              }
          }
      } // end inner class PingTask

}


On 8/16/06, Joe Bowbeer <[hidden email]> wrote:

> At a glance, I notice a few things that could be cleaned up, though
> they may have no impact on the problem.
>
> 1. setFlag is synchronized but there is no synchronized getFlag method.
>
> Either added synchronized getFlag, or declare flag to be "volatile".
>
> 2. doConnect(url) code may fail without closing input stream.
>
> Add try-catch after opening input stream.
>
> 3. Also, I would consider switching to fixed delay to avoid the
> possibility of multiple outstanding pings.
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

tpeierls
On 8/17/06, robert lazarski <[hidden email]> wrote:
Thanks Dave and Joe. I implemented the changes you two mentioned (I
hope) and now invoke a FutureTask inside the Runnable of the
ScheduledFuture .

Maybe instead of creating a new thread for each PingFuture, you could decouple the execution policy of PingFuture from the scheduling of PingTask using an ExecutorService. Specifically, instead of the following code in PingTask.run:

FutureTask<?> f = new FutureTask<Object>(new PingFuture(), null);
Thread thread = new Thread(f);
thread.start();
// 5 seconds to finish connect or will timeout
f.get(5000, TimeUnit.MILLISECONDS);

You could write:

    exec.submit(new PingFuture()).get(5000, TimeUnit.MILLISECONDS);

where exec is initialized with Executors.newCachedThreadPool().

--tim

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

David Holmes-3
In reply to this post by iksrazal
Robert,

A concern with this is what happens to the thread when the connection does
not respond? You have the get() timeout but the thread you used (whether
created directly or via an Executor) may still get hung on the actual
operation. You might want to cancel/interrupt the thread after you timeout.

Cheers,
David Holmes

> -----Original Message-----
> From: [hidden email]
> [mailto:[hidden email]]On Behalf Of robert
> lazarski
> Sent: Friday, 18 August 2006 12:56 AM
> To: Joe Bowbeer
> Cc: [hidden email]
> Subject: Re: [concurrency-interest] Simple ScheduledFuture problem
>
>
> Thanks Dave and Joe. I implemented the changes you two mentioned (I
> hope) and now invoke a FutureTask inside the Runnable of the
> ScheduledFuture . Here's what I came up with. The purpose here is just
> to simply record the states of an app server.
>
> Thanks!
> Robert
>
> package org;
>
> import java.io.BufferedReader;
> import java.io.IOException;
> import java.io.InputStream;
> import java.io.InputStreamReader;
> import java.net.InetAddress;
> import java.net.Socket;
> import java.net.URL;
> import java.net.URLConnection;
> import java.util.HashMap;
> import java.util.Map;
> import java.util.concurrent.Executors;
> import java.util.concurrent.FutureTask;
> import java.util.concurrent.ScheduledExecutorService;
> import java.util.concurrent.ScheduledFuture;
> import java.util.concurrent.TimeUnit;
>
> import org.apache.commons.logging.Log;
> import org.apache.commons.logging.LogFactory;
>
> public class Ping {
>
>       private volatile int  flag = 0;
>       private static int STOPPED = new Integer(0);
>       private static int BEGIN_INIT = new Integer(1);
>       private static int RUNNING = new Integer(2);
>           private static Map <Integer, String> map;
>
>       /** commons logging declaration. */
>       private static Log logger = LogFactory.getLog(
>                 Ping.class);
>
>       public static void main(String[] args) throws Exception {
>
>               new Ping();
>       }
>
>       Ping () {
>           ScheduledExecutorService ses =
> Executors.newSingleThreadScheduledExecutor();
>           // Do pings, starting now, with a 2 second delay in between
>           ScheduledFuture <?> ping =
> ses.scheduleWithFixedDelay(new PingTask(),
>               0L, 2000L, TimeUnit.MILLISECONDS);
>       }
>
>       static {
>           map = new HashMap<Integer, String>();
>           map.put(STOPPED, "STOPPED");
>           map.put(BEGIN_INIT, "BEGIN_INIT");
>           map.put(RUNNING, "RUNNING");
>       }
>
>       private synchronized void setFlag(int var) {
>           flag = var;
>       }
>
>       /** Get message via an int.
>        * @param code Integer mapped to message
>        * @return String mapped message
>        */
>       public final String getMessage(int code) {
>           return map.get(code);
>       }
>
>       class PingFuture implements Runnable {
>           private void doConnect(String host, int port, boolean
> on_connect, int state) {
>               try {
>                   logger.debug("connecting to host: " +host+ ",
> port: " + port);
>                   InetAddress addr = InetAddress.getByName(host);
>                   // will throw an exception if could not connect
>                   Socket s = new Socket(addr, port);
>                   s.close();
>                   if (on_connect) {
>                       logger.debug("Found port: " + port);
>                       setFlag(state);
>                   }
>
>               } catch (Exception ex) {
>                   logger.error("Can't find port: " + port);
>                   logger.error(ex.getMessage(), ex);
>                   if (!on_connect) {
>                       setFlag(state);
>                   }
>               }
>
>           }
>
>           private void doConnect(URL host, boolean on_connect, int
> state) throws IOException {
>               InputStream is = null;
>               BufferedReader in = null;
>               try {
>                   logger.debug("connecting to url: " + host.toString());
>                   URLConnection uc = host.openConnection();
>                   if (uc == null) {
>                       logger.error("Got a null URLConnection object!");
>                       return;
>                   }
>                   is = uc.getInputStream();
>                   if (is == null) {
>                       logger.error("Got a null content object!");
>                       return;
>                   }
>                   in = new BufferedReader(new InputStreamReader(
>                         is));
>                   String inputLine;
>                   // just test that its readable for now
>                   while ((inputLine = in.readLine()) != null)  {
>                         ;
>                   }
>                   if (on_connect) {
>                       logger.debug("Found url: " + host.toString());
>                       setFlag(state);
>                   }
>
>               } catch (Exception ex) {
>                   logger.error("Can't find url: " + host.toString());
>                   logger.error(ex.getMessage(), ex);
>                   if (!on_connect) {
>                       setFlag(state);
>                   }
>               } finally {
>                   if (is != null) {
>                       is.close();
>                   }
>                   if (in != null) {
>                       in.close();
>                   }
>               }
>
>           }
>
>           public void run() {
>
>               try {
>                   if (flag == STOPPED) {
>                       doConnect("localhost", 1099, true, BEGIN_INIT);
>                   }
>                   if (flag == BEGIN_INIT) {
>                       // test state of stopped to prevent endless loop
>                       doConnect("localhost", 1099, false, STOPPED);
>                       doConnect(new
> URL("http://localhost:8080/maragato/"), true, RUNNING);
>
>                   }
>                   if (flag == RUNNING) {
>                       doConnect(new
> URL("http://localhost:8080/maragato/"), false, STOPPED);
>                   }
>
>               } catch (Exception ex) {
>                     logger.error(ex.getMessage(), ex);
>               }
>           }
>       } // end inner class PingFuture
>
>       class PingTask implements Runnable {
>
>           public void run() {
>
>               try {
>                   FutureTask<?> f = new FutureTask<Object>(new
> PingFuture(), null);
>                   Thread thread = new Thread(f);
>                   thread.start();
>                   // 5 seconds to finish connect or will timeout
>                   f.get(5000, TimeUnit.MILLISECONDS);
>               } catch (Exception ex) {
>                     logger.error(ex.getMessage(), ex);
>               }
>               finally {
>                   logger.debug("CURRENT STATE: " + getMessage(flag));
>
>               }
>           }
>       } // end inner class PingTask
>
> }
>
>
> On 8/16/06, Joe Bowbeer <[hidden email]> wrote:
> > At a glance, I notice a few things that could be cleaned up, though
> > they may have no impact on the problem.
> >
> > 1. setFlag is synchronized but there is no synchronized getFlag method.
> >
> > Either added synchronized getFlag, or declare flag to be "volatile".
> >
> > 2. doConnect(url) code may fail without closing input stream.
> >
> > Add try-catch after opening input stream.
> >
> > 3. Also, I would consider switching to fixed delay to avoid the
> > possibility of multiple outstanding pings.
> > _______________________________________________
> > Concurrency-interest mailing list
> > [hidden email]
> > http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
> >
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

iksrazal
I finally got some time to implement the latest suggestions of David
and Tim. This is what I came up with:

class PingTask implements Runnable {

         public void run() {

             Future f = null;
             boolean interrupted = false;
             try {
                 ExecutorService exec = Executors.newCachedThreadPool();
                 f = exec.submit(new PingFuture());
                 f.get(5000, TimeUnit.MILLISECONDS);

             } catch (TimeoutException ex) {
                   interrupted = true;
                   logger.error("Future timed out: \n" + ex.getMessage(), ex);
             } catch (InterruptedException ex) {
                   interrupted = true;
                   logger.error("Future interrupted: \n" + ex.getMessage(), ex);
             } catch (Exception ex) {
                   logger.error(ex.getMessage(), ex);
             }
             finally {
                 f.cancel(true);
                 logger.debug("CURRENT STATE: " + getMessage(flag));

             }
             if (interrupted) {
                 Thread.currentThread().interrupt();
             }
         }
     } // end inner class PingTask

Thanks for the feedback!
Robert

On 8/17/06, David Holmes <[hidden email]> wrote:

> Robert,
>
> A concern with this is what happens to the thread when the connection does
> not respond? You have the get() timeout but the thread you used (whether
> created directly or via an Executor) may still get hung on the actual
> operation. You might want to cancel/interrupt the thread after you timeout.
>
> Cheers,
> David Holmes
>
> > -----Original Message-----
> > From: [hidden email]
> > [mailto:[hidden email]]On Behalf Of robert
> > lazarski
> > Sent: Friday, 18 August 2006 12:56 AM
> > To: Joe Bowbeer
> > Cc: [hidden email]
> > Subject: Re: [concurrency-interest] Simple ScheduledFuture problem
> >
> >
> > Thanks Dave and Joe. I implemented the changes you two mentioned (I
> > hope) and now invoke a FutureTask inside the Runnable of the
> > ScheduledFuture . Here's what I came up with. The purpose here is just
> > to simply record the states of an app server.
> >
> > Thanks!
> > Robert
> >
> > package org;
> >
> > import java.io.BufferedReader;
> > import java.io.IOException;
> > import java.io.InputStream;
> > import java.io.InputStreamReader;
> > import java.net.InetAddress;
> > import java.net.Socket;
> > import java.net.URL;
> > import java.net.URLConnection;
> > import java.util.HashMap;
> > import java.util.Map;
> > import java.util.concurrent.Executors;
> > import java.util.concurrent.FutureTask;
> > import java.util.concurrent.ScheduledExecutorService;
> > import java.util.concurrent.ScheduledFuture;
> > import java.util.concurrent.TimeUnit;
> >
> > import org.apache.commons.logging.Log;
> > import org.apache.commons.logging.LogFactory;
> >
> > public class Ping {
> >
> >       private volatile int  flag = 0;
> >       private static int STOPPED = new Integer(0);
> >       private static int BEGIN_INIT = new Integer(1);
> >       private static int RUNNING = new Integer(2);
> >           private static Map <Integer, String> map;
> >
> >       /** commons logging declaration. */
> >       private static Log logger = LogFactory.getLog(
> >                 Ping.class);
> >
> >       public static void main(String[] args) throws Exception {
> >
> >               new Ping();
> >       }
> >
> >       Ping () {
> >           ScheduledExecutorService ses =
> > Executors.newSingleThreadScheduledExecutor();
> >           // Do pings, starting now, with a 2 second delay in between
> >           ScheduledFuture <?> ping =
> > ses.scheduleWithFixedDelay(new PingTask(),
> >               0L, 2000L, TimeUnit.MILLISECONDS);
> >       }
> >
> >       static {
> >           map = new HashMap<Integer, String>();
> >           map.put(STOPPED, "STOPPED");
> >           map.put(BEGIN_INIT, "BEGIN_INIT");
> >           map.put(RUNNING, "RUNNING");
> >       }
> >
> >       private synchronized void setFlag(int var) {
> >           flag = var;
> >       }
> >
> >       /** Get message via an int.
> >        * @param code Integer mapped to message
> >        * @return String mapped message
> >        */
> >       public final String getMessage(int code) {
> >           return map.get(code);
> >       }
> >
> >       class PingFuture implements Runnable {
> >           private void doConnect(String host, int port, boolean
> > on_connect, int state) {
> >               try {
> >                   logger.debug("connecting to host: " +host+ ",
> > port: " + port);
> >                   InetAddress addr = InetAddress.getByName(host);
> >                   // will throw an exception if could not connect
> >                   Socket s = new Socket(addr, port);
> >                   s.close();
> >                   if (on_connect) {
> >                       logger.debug("Found port: " + port);
> >                       setFlag(state);
> >                   }
> >
> >               } catch (Exception ex) {
> >                   logger.error("Can't find port: " + port);
> >                   logger.error(ex.getMessage(), ex);
> >                   if (!on_connect) {
> >                       setFlag(state);
> >                   }
> >               }
> >
> >           }
> >
> >           private void doConnect(URL host, boolean on_connect, int
> > state) throws IOException {
> >               InputStream is = null;
> >               BufferedReader in = null;
> >               try {
> >                   logger.debug("connecting to url: " + host.toString());
> >                   URLConnection uc = host.openConnection();
> >                   if (uc == null) {
> >                       logger.error("Got a null URLConnection object!");
> >                       return;
> >                   }
> >                   is = uc.getInputStream();
> >                   if (is == null) {
> >                       logger.error("Got a null content object!");
> >                       return;
> >                   }
> >                   in = new BufferedReader(new InputStreamReader(
> >                         is));
> >                   String inputLine;
> >                   // just test that its readable for now
> >                   while ((inputLine = in.readLine()) != null)  {
> >                         ;
> >                   }
> >                   if (on_connect) {
> >                       logger.debug("Found url: " + host.toString());
> >                       setFlag(state);
> >                   }
> >
> >               } catch (Exception ex) {
> >                   logger.error("Can't find url: " + host.toString());
> >                   logger.error(ex.getMessage(), ex);
> >                   if (!on_connect) {
> >                       setFlag(state);
> >                   }
> >               } finally {
> >                   if (is != null) {
> >                       is.close();
> >                   }
> >                   if (in != null) {
> >                       in.close();
> >                   }
> >               }
> >
> >           }
> >
> >           public void run() {
> >
> >               try {
> >                   if (flag == STOPPED) {
> >                       doConnect("localhost", 1099, true, BEGIN_INIT);
> >                   }
> >                   if (flag == BEGIN_INIT) {
> >                       // test state of stopped to prevent endless loop
> >                       doConnect("localhost", 1099, false, STOPPED);
> >                       doConnect(new
> > URL("http://localhost:8080/maragato/"), true, RUNNING);
> >
> >                   }
> >                   if (flag == RUNNING) {
> >                       doConnect(new
> > URL("http://localhost:8080/maragato/"), false, STOPPED);
> >                   }
> >
> >               } catch (Exception ex) {
> >                     logger.error(ex.getMessage(), ex);
> >               }
> >           }
> >       } // end inner class PingFuture
> >
> >       class PingTask implements Runnable {
> >
> >           public void run() {
> >
> >               try {
> >                   FutureTask<?> f = new FutureTask<Object>(new
> > PingFuture(), null);
> >                   Thread thread = new Thread(f);
> >                   thread.start();
> >                   // 5 seconds to finish connect or will timeout
> >                   f.get(5000, TimeUnit.MILLISECONDS);
> >               } catch (Exception ex) {
> >                     logger.error(ex.getMessage(), ex);
> >               }
> >               finally {
> >                   logger.debug("CURRENT STATE: " + getMessage(flag));
> >
> >               }
> >           }
> >       } // end inner class PingTask
> >
> > }
> >
> >
> > On 8/16/06, Joe Bowbeer <[hidden email]> wrote:
> > > At a glance, I notice a few things that could be cleaned up, though
> > > they may have no impact on the problem.
> > >
> > > 1. setFlag is synchronized but there is no synchronized getFlag method.
> > >
> > > Either added synchronized getFlag, or declare flag to be "volatile".
> > >
> > > 2. doConnect(url) code may fail without closing input stream.
> > >
> > > Add try-catch after opening input stream.
> > >
> > > 3. Also, I would consider switching to fixed delay to avoid the
> > > possibility of multiple outstanding pings.
> > > _______________________________________________
> > > Concurrency-interest mailing list
> > > [hidden email]
> > > http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
> > >
> > _______________________________________________
> > Concurrency-interest mailing list
> > [hidden email]
> > http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

tpeierls
You are creating a new thread pool each time the PingTask runs. I was suggesting that the thread pool be a field, maybe even a static field, of PingTask.

--tim

On 8/22/06, robert lazarski <[hidden email]> wrote:
I finally got some time to implement the latest suggestions of David
and Tim. This is what I came up with:

class PingTask implements Runnable {

         public void run() {

             Future f = null;
             boolean interrupted = false;
             try {
                 ExecutorService exec = Executors.newCachedThreadPool();
                 f = exec.submit(new PingFuture());
                 f.get(5000, TimeUnit.MILLISECONDS);

             } catch (TimeoutException ex) {
                   interrupted = true;
                   logger.error("Future timed out: \n" + ex.getMessage(), ex);
             } catch (InterruptedException ex) {
                   interrupted = true;
                   logger.error ("Future interrupted: \n" + ex.getMessage(), ex);
             } catch (Exception ex) {
                   logger.error(ex.getMessage(), ex);
             }
             finally {
                 f.cancel(true);
                 logger.debug("CURRENT STATE: " + getMessage(flag));

             }
             if (interrupted) {
                 Thread.currentThread().interrupt();
             }
         }
     } // end inner class PingTask

Thanks for the feedback!
Robert

On 8/17/06, David Holmes <[hidden email]> wrote:

> Robert,
>
> A concern with this is what happens to the thread when the connection does
> not respond? You have the get() timeout but the thread you used (whether
> created directly or via an Executor) may still get hung on the actual
> operation. You might want to cancel/interrupt the thread after you timeout.
>
> Cheers,
> David Holmes
>
> > -----Original Message-----
> > From: [hidden email]
> > [mailto:[hidden email]]On Behalf Of robert
> > lazarski
> > Sent: Friday, 18 August 2006 12:56 AM
> > To: Joe Bowbeer
> > Cc: [hidden email]
> > Subject: Re: [concurrency-interest] Simple ScheduledFuture problem
> >

> >
> > Thanks Dave and Joe. I implemented the changes you two mentioned (I
> > hope) and now invoke a FutureTask inside the Runnable of the
> > ScheduledFuture . Here's what I came up with. The purpose here is just
> > to simply record the states of an app server.
> >
> > Thanks!
> > Robert
> >
> > package org;
> >
> > import java.io.BufferedReader;
> > import java.io.IOException;
> > import java.io.InputStream;
> > import java.io.InputStreamReader;
> > import java.net.InetAddress;
> > import java.net.Socket;
> > import java.net.URL ;
> > import java.net.URLConnection;
> > import java.util.HashMap;
> > import java.util.Map;
> > import java.util.concurrent.Executors;
> > import java.util.concurrent.FutureTask ;
> > import java.util.concurrent.ScheduledExecutorService;
> > import java.util.concurrent.ScheduledFuture;
> > import java.util.concurrent.TimeUnit;
> >
> > import org.apache.commons.logging.Log ;
> > import org.apache.commons.logging.LogFactory;
> >
> > public class Ping {
> >
> >       private volatile int  flag = 0;
> >       private static int STOPPED = new Integer(0);
> >       private static int BEGIN_INIT = new Integer(1);
> >       private static int RUNNING = new Integer(2);
> >           private static Map <Integer, String> map;
> >
> >       /** commons logging declaration. */
> >       private static Log logger = LogFactory.getLog(
> >                 Ping.class);
> >
> >       public static void main(String[] args) throws Exception {
> >
> >               new Ping();
> >       }
> >
> >       Ping () {
> >           ScheduledExecutorService ses =
> > Executors.newSingleThreadScheduledExecutor();
> >           // Do pings, starting now, with a 2 second delay in between
> >           ScheduledFuture <?> ping =
> > ses.scheduleWithFixedDelay(new PingTask(),
> >               0L, 2000L, TimeUnit.MILLISECONDS);
> >       }
> >
> >       static {
> >           map = new HashMap<Integer, String>();
> >           map.put(STOPPED, "STOPPED");
> >           map.put(BEGIN_INIT, "BEGIN_INIT");
> >           map.put(RUNNING, "RUNNING");
> >       }
> >
> >       private synchronized void setFlag(int var) {
> >           flag = var;
> >       }
> >
> >       /** Get message via an int.
> >        * @param code Integer mapped to message
> >        * @return String mapped message
> >        */
> >       public final String getMessage(int code) {
> >           return map.get(code);
> >       }
> >
> >       class PingFuture implements Runnable {
> >           private void doConnect(String host, int port, boolean
> > on_connect, int state) {
> >               try {
> >                   logger.debug("connecting to host: " +host+ ",
> > port: " + port);
> >                   InetAddress addr = InetAddress.getByName (host);
> >                   // will throw an exception if could not connect
> >                   Socket s = new Socket(addr, port);
> >                   s.close();
> >                   if (on_connect) {
> >                       logger.debug("Found port: " + port);
> >                       setFlag(state);
> >                   }
> >
> >               } catch (Exception ex) {
> >                   logger.error("Can't find port: " + port);
> >                   logger.error(ex.getMessage(), ex);
> >                   if (!on_connect) {
> >                       setFlag(state);
> >                   }
> >               }
> >
> >           }
> >
> >           private void doConnect(URL host, boolean on_connect, int
> > state) throws IOException {
> >               InputStream is = null;
> >               BufferedReader in = null;
> >               try {
> >                   logger.debug("connecting to url: " + host.toString ());
> >                   URLConnection uc = host.openConnection();
> >                   if (uc == null) {
> >                       logger.error("Got a null URLConnection object!");
> >                       return;
> >                   }
> >                   is = uc.getInputStream();
> >                   if (is == null) {
> >                       logger.error ("Got a null content object!");
> >                       return;
> >                   }
> >                   in = new BufferedReader(new InputStreamReader(
> >                         is));
> >                   String inputLine;
> >                   // just test that its readable for now
> >                   while ((inputLine = in.readLine()) != null)  {
> >                         ;
> >                   }
> >                   if (on_connect) {
> >                       logger.debug("Found url: " + host.toString());
> >                       setFlag(state);
> >                   }
> >
> >               } catch (Exception ex) {
> >                   logger.error("Can't find url: " + host.toString());
> >                   logger.error(ex.getMessage(), ex);
> >                   if (!on_connect) {
> >                       setFlag(state);
> >                   }
> >               } finally {
> >                   if (is != null) {
> >                       is.close();
> >                   }
> >                   if (in != null) {
> >                       in.close();
> >                   }
> >               }
> >
> >           }
> >
> >           public void run() {
> >
> >               try {
> >                   if (flag == STOPPED) {
> >                       doConnect("localhost", 1099, true, BEGIN_INIT);
> >                   }
> >                   if (flag == BEGIN_INIT) {
> >                       // test state of stopped to prevent endless loop
> >                       doConnect("localhost", 1099, false, STOPPED);
> >                       doConnect(new
> > URL("<a href="http://localhost:8080/maragato/" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">http://localhost:8080/maragato/ "), true, RUNNING);
> >
> >                   }
> >                   if (flag == RUNNING) {
> >                       doConnect(new
> > URL("<a href="http://localhost:8080/maragato/" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)"> http://localhost:8080/maragato/"), false, STOPPED);
> >                   }
> >
> >               } catch (Exception ex) {
> >                     logger.error(ex.getMessage(), ex);
> >               }
> >           }
> >       } // end inner class PingFuture
> >
> >       class PingTask implements Runnable {
> >
> >           public void run() {
> >
> >               try {
> >                   FutureTask<?> f = new FutureTask<Object>(new

> > PingFuture(), null);
> >                   Thread thread = new Thread(f);
> >                   thread.start ();
> >                   // 5 seconds to finish connect or will timeout
> >                   f.get(5000, TimeUnit.MILLISECONDS);
> >               } catch (Exception ex) {
> >                     logger.error(ex.getMessage(), ex);
> >               }
> >               finally {
> >                   logger.debug("CURRENT STATE: " + getMessage(flag));
> >
> >               }
> >           }
> >       } // end inner class PingTask
> >
> > }
> >
> >
> > On 8/16/06, Joe Bowbeer <[hidden email]> wrote:
> > > At a glance, I notice a few things that could be cleaned up, though
> > > they may have no impact on the problem.
> > >
> > > 1. setFlag is synchronized but there is no synchronized getFlag method.
> > >
> > > Either added synchronized getFlag, or declare flag to be "volatile".
> > >
> > > 2. doConnect(url) code may fail without closing input stream.
> > >
> > > Add try-catch after opening input stream.
> > >
> > > 3. Also, I would consider switching to fixed delay to avoid the
> > > possibility of multiple outstanding pings.
> > > _______________________________________________
> > > Concurrency-interest mailing list
> > > [hidden email]
> > > <a href="http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
> > >
> > _______________________________________________
> > Concurrency-interest mailing list
> > [hidden email]
> > <a href="http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)"> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
<a href="http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

tpeierls
Also, instead of catching Exception, you can catch the more specific ExecutionException -- thrown by Future.get() -- and examine the underlying Throwable with getCause().

--tim

On 8/22/06, robert lazarski <[hidden email]> wrote:
I finally got some time to implement the latest suggestions of David
and Tim. This is what I came up with:

class PingTask implements Runnable {

         public void run() {

             Future f = null;
             boolean interrupted = false;
             try {
                 ExecutorService exec = Executors.newCachedThreadPool();
                 f = exec.submit(new PingFuture());
                 f.get(5000, TimeUnit.MILLISECONDS);

             } catch (TimeoutException ex) {
                   interrupted = true;
                   logger.error("Future timed out: \n" + ex.getMessage(), ex);
             } catch (InterruptedException ex) {
                   interrupted = true;
                   logger.error ("Future interrupted: \n" + ex.getMessage(), ex);
             } catch (Exception ex) {
                   logger.error(ex.getMessage(), ex);
             }
             finally {
                 f.cancel(true);
                 logger.debug("CURRENT STATE: " + getMessage(flag));

             }
             if (interrupted) {
                 Thread.currentThread().interrupt();
             }
         }
     } // end inner class PingTask

Thanks for the feedback!
Robert

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

iksrazal
Thanks Tim. If its any consolation, I did buy the concurrency in
practice book and I'm reading it every day, though I just started ;-)
. PingTask is an inner class and can't have static vars , so this is
the entire new version. Thanks!

package org;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.InetAddress;
import java.net.Socket;
import java.net.URL;
import java.net.URLConnection;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.Executors;
import java.util.concurrent.FutureTask;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.CancellationException;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

public class Ping {

     // for the inner class PingTask
     static ExecutorService exec = Executors.newCachedThreadPool();
     private volatile int  flag = 0;
     private static int STOPPED = new Integer(0);
     private static int BEGIN_INIT = new Integer(1);
     private static int RUNNING = new Integer(2);
         private static Map <Integer, String> map;

     /** commons logging declaration. */
     private static Log logger = LogFactory.getLog(
               Ping.class);

     public static void main(String[] args) throws Exception {

             new Ping();
     }

     Ping () {
         ScheduledExecutorService ses =
Executors.newSingleThreadScheduledExecutor();
         // Do pings, starting now, with a 2 second delay in between
         ScheduledFuture <?> ping = ses.scheduleWithFixedDelay(new PingTask(),
             0L, 2000L, TimeUnit.MILLISECONDS);
     }

     static {
         map = new HashMap<Integer, String>();
         map.put(STOPPED, "STOPPED");
         map.put(BEGIN_INIT, "BEGIN_INIT");
         map.put(RUNNING, "RUNNING");
     }

     private synchronized void setFlag(int var) {
         flag = var;
     }

     /** Get message via an int.
      * @param code Integer mapped to message
      * @return String mapped message
      */
     public final String getMessage(int code) {
         return map.get(code);
     }

     class PingFuture implements Runnable {
         private void doConnect(String host, int port, boolean
on_connect, int state) {
             try {
                 logger.debug("connecting to host: " +host+ ", port: " + port);
                 InetAddress addr = InetAddress.getByName(host);
                 // will throw an exception if could not connect
                 Socket s = new Socket(addr, port);
                 s.close();
                 if (on_connect) {
                     logger.debug("Found port: " + port);
                     setFlag(state);
                 }

             } catch (Exception ex) {
                 logger.error("Can't find port: " + port);
                 logger.error(ex.getMessage(), ex);
                 if (!on_connect) {
                     setFlag(state);
                 }
             }

         }

         private void doConnect(URL host, boolean on_connect, int
state) throws IOException {
             InputStream is = null;
             BufferedReader in = null;
             try {
                 logger.debug("connecting to url: " + host.toString());
                 URLConnection uc = host.openConnection();
                 if (uc == null) {
                     logger.error("Got a null URLConnection object!");
                     return;
                 }
                 is = uc.getInputStream();
                 if (is == null) {
                     logger.error("Got a null content object!");
                     return;
                 }
                 in = new BufferedReader(new InputStreamReader(
                       is));
                 String inputLine;
                 // just test that its readable for now
                 while ((inputLine = in.readLine()) != null)  {
                       ;
                 }
                 if (on_connect) {
                     logger.debug("Found url: " + host.toString());
                     setFlag(state);
                 }

             } catch (Exception ex) {
                 logger.error("Can't find url: " + host.toString());
                 logger.error(ex.getMessage(), ex);
                 if (!on_connect) {
                     setFlag(state);
                 }
             } finally {
                 if (is != null) {
                     is.close();
                 }
                 if (in != null) {
                     in.close();
                 }
             }

         }

         public void run() {

             try {
                 if (flag == STOPPED) {
                     doConnect("localhost", 1099, true, BEGIN_INIT);
                 }
                 if (flag == BEGIN_INIT) {
                     // test state of stopped to prevent endless loop
                     doConnect("localhost", 1099, false, STOPPED);
                     doConnect(new
URL("http://localhost:8080/maragato/"), true, RUNNING);

                 }
                 if (flag == RUNNING) {
                     doConnect(new
URL("http://localhost:8080/maragato/"), false, STOPPED);
                 }

             } catch (Exception ex) {
                   logger.error(ex.getMessage(), ex);
             }
         }
     } // end inner class PingFuture

     class PingTask implements Runnable {


         public void run() {

             Future f = null;
             boolean interrupted = false;
             try {
                 f = exec.submit(new PingFuture());
                 f.get(5000, TimeUnit.MILLISECONDS);

             } catch (ExecutionException ex) {
                   interrupted = true;
                   logger.error("Future threw an error: \n" +
ex.getCause(), ex);
             } catch (CancellationException ex) {
                   interrupted = true;
                   logger.error("Future cancelled: \n" + ex.getCause(), ex);
             } catch (TimeoutException ex) {
                   interrupted = true;
                   logger.error("Future timed out: \n" + ex.getCause(), ex);
             } catch (InterruptedException ex) {
                   interrupted = true;
                   logger.error("Future interrupted: \n" + ex.getCause(), ex);
             } catch (Exception ex) {
                   logger.error("Unexpected error: \n" + ex.getMessage(), ex);
             }
             finally {
                 f.cancel(true);
                 logger.debug("CURRENT STATE: " + getMessage(flag));

             }
             if (interrupted) {
                 Thread.currentThread().interrupt();
             }
         }
     } // end inner class PingTask
}


On 8/22/06, Tim Peierls <[hidden email]> wrote:


On 8/22/06, Tim Peierls <[hidden email]> wrote:

> Also, instead of catching Exception, you can catch the more specific
> ExecutionException -- thrown by Future.get() -- and examine the underlying
> Throwable with getCause().
>
>
> --tim
>
>
> >
> >
> > On 8/22/06, robert lazarski <[hidden email]> wrote:
> > > I finally got some time to implement the latest suggestions of David
> > > and Tim. This is what I came up with:
> > >
> > > class PingTask implements Runnable {
> > >
> > >          public void run() {
> > >
> > >              Future f = null;
> > >              boolean interrupted = false;
> > >              try {
> > >                  ExecutorService exec = Executors.newCachedThreadPool();
> > >                  f = exec.submit(new PingFuture());
> > >                  f.get(5000, TimeUnit.MILLISECONDS);
> > >
> > >              } catch (TimeoutException ex) {
> > >                    interrupted = true;
> > >                    logger.error("Future timed out: \n" +
> ex.getMessage(), ex);
> > >              } catch (InterruptedException ex) {
> > >                    interrupted = true;
> > >                    logger.error ("Future interrupted: \n" +
> ex.getMessage(), ex);
> > >              } catch (Exception ex) {
> > >                    logger.error(ex.getMessage(), ex);
> > >              }
> > >              finally {
> > >                  f.cancel(true);
> > >                  logger.debug("CURRENT STATE: " + getMessage(flag));
> > >
> > >              }
> > >              if (interrupted) {
> > >                  Thread.currentThread().interrupt();
> > >              }
> > >          }
> > >      } // end inner class PingTask
> > >
> > > Thanks for the feedback!
> > > Robert
> > >
> >
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

Dhanji R. Prasanna
Im curious, what is the purpose of declaring flag volatile AND
synchronizing setter access to it?

If it is volatile it will (could) be overwritten concurrently in a
visible manner anyway. Afaik the purpose of synchronizing access to a
field is to prevent diverging concurrent states (i.e. multiple states
of the same variable cached in separate threads and merged back
non-deterministically), afaik this is a moot point since the field is
volatile--a read should yield the same value of "flag" for all threads
should it not?

Since there is no happens-before edge established in setFlag(), I fail
to see how synchronizing it does anything useful?

I may very well be missing something here...

On 8/23/06, robert lazarski <[hidden email]> wrote:

> Thanks Tim. If its any consolation, I did buy the concurrency in
> practice book and I'm reading it every day, though I just started ;-)
> . PingTask is an inner class and can't have static vars , so this is
> the entire new version. Thanks!
>
> package org;
>
> import java.io.BufferedReader;
> import java.io.IOException;
> import java.io.InputStream;
> import java.io.InputStreamReader;
> import java.net.InetAddress;
> import java.net.Socket;
> import java.net.URL;
> import java.net.URLConnection;
> import java.util.HashMap;
> import java.util.Map;
> import java.util.concurrent.Executors;
> import java.util.concurrent.FutureTask;
> import java.util.concurrent.ScheduledExecutorService;
> import java.util.concurrent.ScheduledFuture;
> import java.util.concurrent.TimeUnit;
> import java.util.concurrent.ExecutorService;
> import java.util.concurrent.Executors;
> import java.util.concurrent.Future;
> import java.util.concurrent.TimeoutException;
> import java.util.concurrent.ExecutionException;
> import java.util.concurrent.CancellationException;
>
> import org.apache.commons.logging.Log;
> import org.apache.commons.logging.LogFactory;
>
> public class Ping {
>
>      // for the inner class PingTask
>      static ExecutorService exec = Executors.newCachedThreadPool();
>      private volatile int  flag = 0;
>      private static int STOPPED = new Integer(0);
>      private static int BEGIN_INIT = new Integer(1);
>      private static int RUNNING = new Integer(2);
>          private static Map <Integer, String> map;
>
>      /** commons logging declaration. */
>      private static Log logger = LogFactory.getLog(
>                Ping.class);
>
>      public static void main(String[] args) throws Exception {
>
>              new Ping();
>      }
>
>      Ping () {
>          ScheduledExecutorService ses =
> Executors.newSingleThreadScheduledExecutor();
>          // Do pings, starting now, with a 2 second delay in between
>          ScheduledFuture <?> ping = ses.scheduleWithFixedDelay(new PingTask(),
>              0L, 2000L, TimeUnit.MILLISECONDS);
>      }
>
>      static {
>          map = new HashMap<Integer, String>();
>          map.put(STOPPED, "STOPPED");
>          map.put(BEGIN_INIT, "BEGIN_INIT");
>          map.put(RUNNING, "RUNNING");
>      }
>
>      private synchronized void setFlag(int var) {
>          flag = var;
>      }
>
>      /** Get message via an int.
>       * @param code Integer mapped to message
>       * @return String mapped message
>       */
>      public final String getMessage(int code) {
>          return map.get(code);
>      }
>
>      class PingFuture implements Runnable {
>          private void doConnect(String host, int port, boolean
> on_connect, int state) {
>              try {
>                  logger.debug("connecting to host: " +host+ ", port: " + port);
>                  InetAddress addr = InetAddress.getByName(host);
>                  // will throw an exception if could not connect
>                  Socket s = new Socket(addr, port);
>                  s.close();
>                  if (on_connect) {
>                      logger.debug("Found port: " + port);
>                      setFlag(state);
>                  }
>
>              } catch (Exception ex) {
>                  logger.error("Can't find port: " + port);
>                  logger.error(ex.getMessage(), ex);
>                  if (!on_connect) {
>                      setFlag(state);
>                  }
>              }
>
>          }
>
>          private void doConnect(URL host, boolean on_connect, int
> state) throws IOException {
>              InputStream is = null;
>              BufferedReader in = null;
>              try {
>                  logger.debug("connecting to url: " + host.toString());
>                  URLConnection uc = host.openConnection();
>                  if (uc == null) {
>                      logger.error("Got a null URLConnection object!");
>                      return;
>                  }
>                  is = uc.getInputStream();
>                  if (is == null) {
>                      logger.error("Got a null content object!");
>                      return;
>                  }
>                  in = new BufferedReader(new InputStreamReader(
>                        is));
>                  String inputLine;
>                  // just test that its readable for now
>                  while ((inputLine = in.readLine()) != null)  {
>                        ;
>                  }
>                  if (on_connect) {
>                      logger.debug("Found url: " + host.toString());
>                      setFlag(state);
>                  }
>
>              } catch (Exception ex) {
>                  logger.error("Can't find url: " + host.toString());
>                  logger.error(ex.getMessage(), ex);
>                  if (!on_connect) {
>                      setFlag(state);
>                  }
>              } finally {
>                  if (is != null) {
>                      is.close();
>                  }
>                  if (in != null) {
>                      in.close();
>                  }
>              }
>
>          }
>
>          public void run() {
>
>              try {
>                  if (flag == STOPPED) {
>                      doConnect("localhost", 1099, true, BEGIN_INIT);
>                  }
>                  if (flag == BEGIN_INIT) {
>                      // test state of stopped to prevent endless loop
>                      doConnect("localhost", 1099, false, STOPPED);
>                      doConnect(new
> URL("http://localhost:8080/maragato/"), true, RUNNING);
>
>                  }
>                  if (flag == RUNNING) {
>                      doConnect(new
> URL("http://localhost:8080/maragato/"), false, STOPPED);
>                  }
>
>              } catch (Exception ex) {
>                    logger.error(ex.getMessage(), ex);
>              }
>          }
>      } // end inner class PingFuture
>
>      class PingTask implements Runnable {
>
>
>          public void run() {
>
>              Future f = null;
>              boolean interrupted = false;
>              try {
>                  f = exec.submit(new PingFuture());
>                  f.get(5000, TimeUnit.MILLISECONDS);
>
>              } catch (ExecutionException ex) {
>                    interrupted = true;
>                    logger.error("Future threw an error: \n" +
> ex.getCause(), ex);
>              } catch (CancellationException ex) {
>                    interrupted = true;
>                    logger.error("Future cancelled: \n" + ex.getCause(), ex);
>              } catch (TimeoutException ex) {
>                    interrupted = true;
>                    logger.error("Future timed out: \n" + ex.getCause(), ex);
>              } catch (InterruptedException ex) {
>                    interrupted = true;
>                    logger.error("Future interrupted: \n" + ex.getCause(), ex);
>              } catch (Exception ex) {
>                    logger.error("Unexpected error: \n" + ex.getMessage(), ex);
>              }
>              finally {
>                  f.cancel(true);
>                  logger.debug("CURRENT STATE: " + getMessage(flag));
>
>              }
>              if (interrupted) {
>                  Thread.currentThread().interrupt();
>              }
>          }
>      } // end inner class PingTask
> }
>
>
> On 8/22/06, Tim Peierls <[hidden email]> wrote:
>
>
> On 8/22/06, Tim Peierls <[hidden email]> wrote:
> > Also, instead of catching Exception, you can catch the more specific
> > ExecutionException -- thrown by Future.get() -- and examine the underlying
> > Throwable with getCause().
> >
> >
> > --tim
> >
> >
> > >
> > >
> > > On 8/22/06, robert lazarski <[hidden email]> wrote:
> > > > I finally got some time to implement the latest suggestions of David
> > > > and Tim. This is what I came up with:
> > > >
> > > > class PingTask implements Runnable {
> > > >
> > > >          public void run() {
> > > >
> > > >              Future f = null;
> > > >              boolean interrupted = false;
> > > >              try {
> > > >                  ExecutorService exec = Executors.newCachedThreadPool();
> > > >                  f = exec.submit(new PingFuture());
> > > >                  f.get(5000, TimeUnit.MILLISECONDS);
> > > >
> > > >              } catch (TimeoutException ex) {
> > > >                    interrupted = true;
> > > >                    logger.error("Future timed out: \n" +
> > ex.getMessage(), ex);
> > > >              } catch (InterruptedException ex) {
> > > >                    interrupted = true;
> > > >                    logger.error ("Future interrupted: \n" +
> > ex.getMessage(), ex);
> > > >              } catch (Exception ex) {
> > > >                    logger.error(ex.getMessage(), ex);
> > > >              }
> > > >              finally {
> > > >                  f.cancel(true);
> > > >                  logger.debug("CURRENT STATE: " + getMessage(flag));
> > > >
> > > >              }
> > > >              if (interrupted) {
> > > >                  Thread.currentThread().interrupt();
> > > >              }
> > > >          }
> > > >      } // end inner class PingTask
> > > >
> > > > Thanks for the feedback!
> > > > Robert
> > > >
> > >
> >
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

iksrazal
It was due to advice from Joe BowBeer:

" 1. setFlag is synchronized but there is no synchronized getFlag method.

Either added synchronized getFlag, or declare flag to be "volatile". "

Robert

On 8/22/06, Dhanji R. Prasanna <[hidden email]> wrote:

> Im curious, what is the purpose of declaring flag volatile AND
> synchronizing setter access to it?
>
> If it is volatile it will (could) be overwritten concurrently in a
> visible manner anyway. Afaik the purpose of synchronizing access to a
> field is to prevent diverging concurrent states (i.e. multiple states
> of the same variable cached in separate threads and merged back
> non-deterministically), afaik this is a moot point since the field is
> volatile--a read should yield the same value of "flag" for all threads
> should it not?
>
> Since there is no happens-before edge established in setFlag(), I fail
> to see how synchronizing it does anything useful?
>
> I may very well be missing something here...
>
> On 8/23/06, robert lazarski <[hidden email]> wrote:
> > Thanks Tim. If its any consolation, I did buy the concurrency in
> > practice book and I'm reading it every day, though I just started ;-)
> > . PingTask is an inner class and can't have static vars , so this is
> > the entire new version. Thanks!
> >
> > package org;
> >
> > import java.io.BufferedReader;
> > import java.io.IOException;
> > import java.io.InputStream;
> > import java.io.InputStreamReader;
> > import java.net.InetAddress;
> > import java.net.Socket;
> > import java.net.URL;
> > import java.net.URLConnection;
> > import java.util.HashMap;
> > import java.util.Map;
> > import java.util.concurrent.Executors;
> > import java.util.concurrent.FutureTask;
> > import java.util.concurrent.ScheduledExecutorService;
> > import java.util.concurrent.ScheduledFuture;
> > import java.util.concurrent.TimeUnit;
> > import java.util.concurrent.ExecutorService;
> > import java.util.concurrent.Executors;
> > import java.util.concurrent.Future;
> > import java.util.concurrent.TimeoutException;
> > import java.util.concurrent.ExecutionException;
> > import java.util.concurrent.CancellationException;
> >
> > import org.apache.commons.logging.Log;
> > import org.apache.commons.logging.LogFactory;
> >
> > public class Ping {
> >
> >      // for the inner class PingTask
> >      static ExecutorService exec = Executors.newCachedThreadPool();
> >      private volatile int  flag = 0;
> >      private static int STOPPED = new Integer(0);
> >      private static int BEGIN_INIT = new Integer(1);
> >      private static int RUNNING = new Integer(2);
> >          private static Map <Integer, String> map;
> >
> >      /** commons logging declaration. */
> >      private static Log logger = LogFactory.getLog(
> >                Ping.class);
> >
> >      public static void main(String[] args) throws Exception {
> >
> >              new Ping();
> >      }
> >
> >      Ping () {
> >          ScheduledExecutorService ses =
> > Executors.newSingleThreadScheduledExecutor();
> >          // Do pings, starting now, with a 2 second delay in between
> >          ScheduledFuture <?> ping = ses.scheduleWithFixedDelay(new PingTask(),
> >              0L, 2000L, TimeUnit.MILLISECONDS);
> >      }
> >
> >      static {
> >          map = new HashMap<Integer, String>();
> >          map.put(STOPPED, "STOPPED");
> >          map.put(BEGIN_INIT, "BEGIN_INIT");
> >          map.put(RUNNING, "RUNNING");
> >      }
> >
> >      private synchronized void setFlag(int var) {
> >          flag = var;
> >      }
> >
> >      /** Get message via an int.
> >       * @param code Integer mapped to message
> >       * @return String mapped message
> >       */
> >      public final String getMessage(int code) {
> >          return map.get(code);
> >      }
> >
> >      class PingFuture implements Runnable {
> >          private void doConnect(String host, int port, boolean
> > on_connect, int state) {
> >              try {
> >                  logger.debug("connecting to host: " +host+ ", port: " + port);
> >                  InetAddress addr = InetAddress.getByName(host);
> >                  // will throw an exception if could not connect
> >                  Socket s = new Socket(addr, port);
> >                  s.close();
> >                  if (on_connect) {
> >                      logger.debug("Found port: " + port);
> >                      setFlag(state);
> >                  }
> >
> >              } catch (Exception ex) {
> >                  logger.error("Can't find port: " + port);
> >                  logger.error(ex.getMessage(), ex);
> >                  if (!on_connect) {
> >                      setFlag(state);
> >                  }
> >              }
> >
> >          }
> >
> >          private void doConnect(URL host, boolean on_connect, int
> > state) throws IOException {
> >              InputStream is = null;
> >              BufferedReader in = null;
> >              try {
> >                  logger.debug("connecting to url: " + host.toString());
> >                  URLConnection uc = host.openConnection();
> >                  if (uc == null) {
> >                      logger.error("Got a null URLConnection object!");
> >                      return;
> >                  }
> >                  is = uc.getInputStream();
> >                  if (is == null) {
> >                      logger.error("Got a null content object!");
> >                      return;
> >                  }
> >                  in = new BufferedReader(new InputStreamReader(
> >                        is));
> >                  String inputLine;
> >                  // just test that its readable for now
> >                  while ((inputLine = in.readLine()) != null)  {
> >                        ;
> >                  }
> >                  if (on_connect) {
> >                      logger.debug("Found url: " + host.toString());
> >                      setFlag(state);
> >                  }
> >
> >              } catch (Exception ex) {
> >                  logger.error("Can't find url: " + host.toString());
> >                  logger.error(ex.getMessage(), ex);
> >                  if (!on_connect) {
> >                      setFlag(state);
> >                  }
> >              } finally {
> >                  if (is != null) {
> >                      is.close();
> >                  }
> >                  if (in != null) {
> >                      in.close();
> >                  }
> >              }
> >
> >          }
> >
> >          public void run() {
> >
> >              try {
> >                  if (flag == STOPPED) {
> >                      doConnect("localhost", 1099, true, BEGIN_INIT);
> >                  }
> >                  if (flag == BEGIN_INIT) {
> >                      // test state of stopped to prevent endless loop
> >                      doConnect("localhost", 1099, false, STOPPED);
> >                      doConnect(new
> > URL("http://localhost:8080/maragato/"), true, RUNNING);
> >
> >                  }
> >                  if (flag == RUNNING) {
> >                      doConnect(new
> > URL("http://localhost:8080/maragato/"), false, STOPPED);
> >                  }
> >
> >              } catch (Exception ex) {
> >                    logger.error(ex.getMessage(), ex);
> >              }
> >          }
> >      } // end inner class PingFuture
> >
> >      class PingTask implements Runnable {
> >
> >
> >          public void run() {
> >
> >              Future f = null;
> >              boolean interrupted = false;
> >              try {
> >                  f = exec.submit(new PingFuture());
> >                  f.get(5000, TimeUnit.MILLISECONDS);
> >
> >              } catch (ExecutionException ex) {
> >                    interrupted = true;
> >                    logger.error("Future threw an error: \n" +
> > ex.getCause(), ex);
> >              } catch (CancellationException ex) {
> >                    interrupted = true;
> >                    logger.error("Future cancelled: \n" + ex.getCause(), ex);
> >              } catch (TimeoutException ex) {
> >                    interrupted = true;
> >                    logger.error("Future timed out: \n" + ex.getCause(), ex);
> >              } catch (InterruptedException ex) {
> >                    interrupted = true;
> >                    logger.error("Future interrupted: \n" + ex.getCause(), ex);
> >              } catch (Exception ex) {
> >                    logger.error("Unexpected error: \n" + ex.getMessage(), ex);
> >              }
> >              finally {
> >                  f.cancel(true);
> >                  logger.debug("CURRENT STATE: " + getMessage(flag));
> >
> >              }
> >              if (interrupted) {
> >                  Thread.currentThread().interrupt();
> >              }
> >          }
> >      } // end inner class PingTask
> > }
> >
> >
> > On 8/22/06, Tim Peierls <[hidden email]> wrote:
> >
> >
> > On 8/22/06, Tim Peierls <[hidden email]> wrote:
> > > Also, instead of catching Exception, you can catch the more specific
> > > ExecutionException -- thrown by Future.get() -- and examine the underlying
> > > Throwable with getCause().
> > >
> > >
> > > --tim
> > >
> > >
> > > >
> > > >
> > > > On 8/22/06, robert lazarski <[hidden email]> wrote:
> > > > > I finally got some time to implement the latest suggestions of David
> > > > > and Tim. This is what I came up with:
> > > > >
> > > > > class PingTask implements Runnable {
> > > > >
> > > > >          public void run() {
> > > > >
> > > > >              Future f = null;
> > > > >              boolean interrupted = false;
> > > > >              try {
> > > > >                  ExecutorService exec = Executors.newCachedThreadPool();
> > > > >                  f = exec.submit(new PingFuture());
> > > > >                  f.get(5000, TimeUnit.MILLISECONDS);
> > > > >
> > > > >              } catch (TimeoutException ex) {
> > > > >                    interrupted = true;
> > > > >                    logger.error("Future timed out: \n" +
> > > ex.getMessage(), ex);
> > > > >              } catch (InterruptedException ex) {
> > > > >                    interrupted = true;
> > > > >                    logger.error ("Future interrupted: \n" +
> > > ex.getMessage(), ex);
> > > > >              } catch (Exception ex) {
> > > > >                    logger.error(ex.getMessage(), ex);
> > > > >              }
> > > > >              finally {
> > > > >                  f.cancel(true);
> > > > >                  logger.debug("CURRENT STATE: " + getMessage(flag));
> > > > >
> > > > >              }
> > > > >              if (interrupted) {
> > > > >                  Thread.currentThread().interrupt();
> > > > >              }
> > > > >          }
> > > > >      } // end inner class PingTask
> > > > >
> > > > > Thanks for the feedback!
> > > > > Robert
> > > > >
> > > >
> > >
> > _______________________________________________
> > Concurrency-interest mailing list
> > [hidden email]
> > http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
> >
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

David Holmes-3
In reply to this post by Dhanji R. Prasanna
> Dhanji R. Prasanna writes:
> Im curious, what is the purpose of declaring flag volatile AND
> synchronizing setter access to it?

It gives you a simple/crude read/write lock.

David Holmes
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

Dhanji R. Prasanna
so the volatile field cant be read when a thread is in its synchronized setter?

On 8/23/06, David Holmes <[hidden email]> wrote:
> > Dhanji R. Prasanna writes:
> > Im curious, what is the purpose of declaring flag volatile AND
> > synchronizing setter access to it?
>
> It gives you a simple/crude read/write lock.
>
> David Holmes
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

David Holmes-3
> so the volatile field cant be read when a thread is in its
> synchronized setter?

Yes it can, but the assignment in the setter is atomic so the semantics are
"as if" the read occurred just before or just after the synchronized
operation.

It's a degenerate case of a read/write lock. You never have to exclude the
writer with respect to the reader because the action of the writer is atomic
with respect to the reader anyway. Of course that breaks down if more than
one assignment were involved.

Cheers,
David Holmes

> On 8/23/06, David Holmes <[hidden email]> wrote:
> > > Dhanji R. Prasanna writes:
> > > Im curious, what is the purpose of declaring flag volatile AND
> > > synchronizing setter access to it?
> >
> > It gives you a simple/crude read/write lock.
> >
> > David Holmes
> >

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

David Holmes-3
I hasten to add that if the setter is simply of the form:

synchronized void set(int newValue) {
   field = newValue;
}

and field is volatile, then making the setter synchronized is unnecessary.

David

> -----Original Message-----
> From: [hidden email]
> [mailto:[hidden email]]On Behalf Of David
> Holmes
> Sent: Wednesday, 23 August 2006 10:13 AM
> To: Dhanji R. Prasanna
> Cc: [hidden email]
> Subject: Re: [concurrency-interest] Simple ScheduledFuture problem
>
>
> > so the volatile field cant be read when a thread is in its
> > synchronized setter?
>
> Yes it can, but the assignment in the setter is atomic so the
> semantics are
> "as if" the read occurred just before or just after the synchronized
> operation.
>
> It's a degenerate case of a read/write lock. You never have to exclude the
> writer with respect to the reader because the action of the
> writer is atomic
> with respect to the reader anyway. Of course that breaks down if more than
> one assignment were involved.
>
> Cheers,
> David Holmes
>
> > On 8/23/06, David Holmes <[hidden email]> wrote:
> > > > Dhanji R. Prasanna writes:
> > > > Im curious, what is the purpose of declaring flag volatile AND
> > > > synchronizing setter access to it?
> > >
> > > It gives you a simple/crude read/write lock.
> > >
> > > David Holmes
> > >
>
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

Dhanji R. Prasanna
On 8/23/06, David Holmes <[hidden email]> wrote:
> I hasten to add that if the setter is simply of the form:
>
> synchronized void set(int newValue) {
>    field = newValue;
> }
>
> and field is volatile, then making the setter synchronized is unnecessary.

This was pretty much my whole argument =)

Dhanji

>
> David
>
> > -----Original Message-----
> > From: [hidden email]
> > [mailto:[hidden email]]On Behalf Of David
> > Holmes
> > Sent: Wednesday, 23 August 2006 10:13 AM
> > To: Dhanji R. Prasanna
> > Cc: [hidden email]
> > Subject: Re: [concurrency-interest] Simple ScheduledFuture problem
> >
> >
> > > so the volatile field cant be read when a thread is in its
> > > synchronized setter?
> >
> > Yes it can, but the assignment in the setter is atomic so the
> > semantics are
> > "as if" the read occurred just before or just after the synchronized
> > operation.
> >
> > It's a degenerate case of a read/write lock. You never have to exclude the
> > writer with respect to the reader because the action of the
> > writer is atomic
> > with respect to the reader anyway. Of course that breaks down if more than
> > one assignment were involved.
> >
> > Cheers,
> > David Holmes
> >
> > > On 8/23/06, David Holmes <[hidden email]> wrote:
> > > > > Dhanji R. Prasanna writes:
> > > > > Im curious, what is the purpose of declaring flag volatile AND
> > > > > synchronizing setter access to it?
> > > >
> > > > It gives you a simple/crude read/write lock.
> > > >
> > > > David Holmes
> > > >
> >
> > _______________________________________________
> > Concurrency-interest mailing list
> > [hidden email]
> > http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

David Holmes-3
Sorry. I focussed on the general question rather then the specific code in
this case.

David

> -----Original Message-----
> From: Dhanji R. Prasanna [mailto:[hidden email]]
> Sent: Wednesday, 23 August 2006 10:40 AM
> To: [hidden email]
> Cc: [hidden email]
> Subject: Re: [concurrency-interest] Simple ScheduledFuture problem
>
>
> On 8/23/06, David Holmes <[hidden email]> wrote:
> > I hasten to add that if the setter is simply of the form:
> >
> > synchronized void set(int newValue) {
> >    field = newValue;
> > }
> >
> > and field is volatile, then making the setter synchronized is
> unnecessary.
>
> This was pretty much my whole argument =)
>
> Dhanji
>
> >
> > David
> >
> > > -----Original Message-----
> > > From: [hidden email]
> > > [mailto:[hidden email]]On Behalf Of David
> > > Holmes
> > > Sent: Wednesday, 23 August 2006 10:13 AM
> > > To: Dhanji R. Prasanna
> > > Cc: [hidden email]
> > > Subject: Re: [concurrency-interest] Simple ScheduledFuture problem
> > >
> > >
> > > > so the volatile field cant be read when a thread is in its
> > > > synchronized setter?
> > >
> > > Yes it can, but the assignment in the setter is atomic so the
> > > semantics are
> > > "as if" the read occurred just before or just after the synchronized
> > > operation.
> > >
> > > It's a degenerate case of a read/write lock. You never have
> to exclude the
> > > writer with respect to the reader because the action of the
> > > writer is atomic
> > > with respect to the reader anyway. Of course that breaks down
> if more than
> > > one assignment were involved.
> > >
> > > Cheers,
> > > David Holmes
> > >
> > > > On 8/23/06, David Holmes <[hidden email]> wrote:
> > > > > > Dhanji R. Prasanna writes:
> > > > > > Im curious, what is the purpose of declaring flag volatile AND
> > > > > > synchronizing setter access to it?
> > > > >
> > > > > It gives you a simple/crude read/write lock.
> > > > >
> > > > > David Holmes
> > > > >
> > >
> > > _______________________________________________
> > > Concurrency-interest mailing list
> > > [hidden email]
> > > http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
> >
> >

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

Brian Goetz
In reply to this post by Dhanji R. Prasanna
>> I hasten to add that if the setter is simply of the form:
>>
>> synchronized void set(int newValue) {
>>    field = newValue;
>> }
>>
>> and field is volatile, then making the setter synchronized is unnecessary.
>
> This was pretty much my whole argument =)

Right.  What David was getting at is the case where you want to do some
sort of nontrivial update, like increment.  The modification
operation(s) use synchronized to make them atomic, and the read
operations use volatile to reduce the cost / improve the scalability
where reads outnumber writes.  Like a read-write lock, but cheaper.

As you've pointed out, this is a very fragile pattern -- if using it,
document it carefully!
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Simple ScheduledFuture problem

Dhanji R. Prasanna
Yep, seems like there's a misunderstanding of volatile/synchronized
going on. Several questions recently have revolved around it.

Thanks David and Brian for  your responses.

On 8/31/06, Brian Goetz <[hidden email]> wrote:

> >> I hasten to add that if the setter is simply of the form:
> >>
> >> synchronized void set(int newValue) {
> >>    field = newValue;
> >> }
> >>
> >> and field is volatile, then making the setter synchronized is unnecessary.
> >
> > This was pretty much my whole argument =)
>
> Right.  What David was getting at is the case where you want to do some
> sort of nontrivial update, like increment.  The modification
> operation(s) use synchronized to make them atomic, and the read
> operations use volatile to reduce the cost / improve the scalability
> where reads outnumber writes.  Like a read-write lock, but cheaper.
>
> As you've pointed out, this is a very fragile pattern -- if using it,
> document it carefully!
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
12