7 Replies Latest reply: Oct 16, 2012 2:03 AM by gimbal2 RSS

    Concurrency help

    802569
      Hi, I'm having some trouble figuring out the specific concurrency issues with the following. My concern is in MyCars getCar(). The parameters will belong to each thread's stack, but is it possible that the car found/returned will be that of another thread and its search parameters? Thanks.
      public class MyServlet extends HttpServlet {
          public static MyCars cars = new MyCars();
          static {
              cars.addCar(1999, 100);
              cars.addCar(2000, 120);
              cars.addCar(2001, 130);
              cars.addCar(2002, 140);
              cars.addCar(2003, 145);
              cars.addCar(2004, 150);
          }
      
      
          public void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
      
              int year = Integer.parseInt(req.getParameter("year"));
              int speed= Integer.parseInt(req.getParameter("speed"));
      
              Garage g = new Garage(cars);
              g.find(year, speed);
      
          }
      }
      
      public class Garage {
          private static MyCars cars;
      
          public Garage(MyCars c) {
              cars = c;
          }
          
          public Car find(int year, int speed) {
              Car car = cars.getCar(year, speed);
              return car;
          }
      }
      
      
      public class MyCars {
          
          private LinkedList<Car> cars = new LinkedList<Car>();
          
          public void addCar(int year, int speed) {
              cars.add(new Car(year, speed));
          }
          
          public Car getCar(int year, int speed) {
              for(Car car : cars) {
                  if(car.getYear() == year && speed == car.getSpeed())
                      return car;
              }
              return null;
          }
      }
        • 1. Re: Concurrency help
          rp0428
          public class Garage {
              private static MyCars cars;
           
              public Garage(MyCars c) {
                  cars = c;
              }
          Why are you overwriting the static 'cars' everytime you create a new instance of Garage?
          • 2. Re: Concurrency help
            802569
            As opposed to creating one instance variable of garage or a static garage? No reason really. This is just the scenario I inherited.
            • 3. Re: Concurrency help
              Kayaman
              799566 wrote:
              but is it possible that the car found/returned will be that of another thread and its search parameters? Thanks.
              No, it's not possible. Each request will make its own Garage (albeit with the static cars property) and happily call the method with its own parameters. No possibility for parameters to start jumping into different threads.

              And I would get rid of those statics there as well. The author doesn't seem to know what the static qualifier means.
              • 4. Re: Concurrency help
                TPD-Opitz
                799566 wrote:
                Hi, I'm having some trouble figuring out the specific concurrency issues with the following. My concern is in MyCars getCar(). The parameters will belong to each thread's stack, but is it possible that the car found/returned will be that of another thread and its search parameters? Thanks.
                For the first shot I'd make the <tt>Garage</tt> a singelton. This will work as long as you use one ServletContainer instance only.
                public class MyServlet extends HttpServlet {
                    // members should be private.
                     private static Garage theGarage = Garage.INSTANCE;
                
                     public void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
                         int year = Integer.parseInt(req.getParameter("year"));
                         int speed= Integer.parseInt(req.getParameter("speed"));
                         theGarage.find(year, speed);
                     }
                 }
                 public class Garage {
                     public static final INSTANCE = new Garage(); // the one and only instance (in this JVM)
                     private final MyCars cars = new MyCars (); // members should be private.
                 
                     private Garage() { // prevent foreing instantiation.
                        // adding dummy cars here, the do not belong to the servelet class
                         cars.addCar(1999, 100);
                         cars.addCar(2000, 120);
                         cars.addCar(2001, 130);
                         cars.addCar(2002, 140);
                         cars.addCar(2003, 145);
                         cars.addCar(2004, 150);
                     }
                      public Car find(int year, int speed) {
                         return  cars.getCar(year, speed); // save a line... ;o)
                     }
                     // you also meight need a delegate for adding cards...
                     public vid addCar(Car pCar){
                        cars.add(pCar);
                     }
                 }
                 
                 
                 public class MyCars {
                     // code against interfaces (cars is a List rather than a LinkedList)
                     private List<Car> cars = new LinkedList<Car>();
                     
                     public void addCar(int year, int speed) {
                         cars.add(new Car(year, speed));
                     }
                     
                     public Car getCar(int year, int speed) {
                         for(Car car : cars) {
                             if(car.getYear() == year && speed == car.getSpeed())
                                 return car;
                         }
                       // never return null unless it's a valid member of the collection.
                       // callers code will look much cleaner if you don't have to check the return for null ...
                       //  alternatively you could return a "dummy car" providing the "not found" message.
                         throw new /*Runtime*/Exception("no car found for year= "+year+" and speed= "+speed);
                     }
                 }
                bye
                TPD
                • 5. Re: Concurrency help
                  gbishop
                  If you are going to create singletons, use the enum pattern, it's easier to use and less complicated.
                  • 6. Re: Concurrency help
                    Kayaman
                    gbishop wrote:
                    If you are going to create singletons, use the enum pattern, it's easier to use and less complicated.
                    You're giving out a lot of obtuse advice. I'd wager he hasn't even got an idea about singletons, and you're already trying to coerce him into your best practices.
                    Not that you shouldn't give out advice here, but your 10 or so posts have included these blatant rules (don't use checked exceptions etc.), yet you're in no way an authority. And you will get called out. And that will result in ugly "conversations" and locked threads and uselessly hurt feelings all around.
                    • 7. Re: Concurrency help
                      gimbal2
                      Kayaman wrote:
                      gbishop wrote:
                      If you are going to create singletons, use the enum pattern, it's easier to use and less complicated.
                      You're giving out a lot of obtuse advice. I'd wager he hasn't even got an idea about singletons, and you're already trying to coerce him into your best practices.
                      Not that you shouldn't give out advice here, but your 10 or so posts have included these blatant rules (don't use checked exceptions etc.), yet you're in no way an authority. And you will get called out. And that will result in ugly "conversations" and locked threads and uselessly hurt feelings all around.
                      Yeah!