Java Concurrency Question

[Ian Norris]

Hi,

I need some help on the a Java interview question on currency.
A friend of mine recently interviewed for a Java position and during the interview, he was presented some code for a fictitious car rental system.
He was asked to identify and fix, among other errors, the concurrency issues with the code. He couldn't quite do it at the interview.
Here is my proposed solution to the problem, but I am not sure if this works and will appreciate some advice.

The code skeleton is given below (some details are omitted for brevity):

Given classes:

Car  {
	String registration;
	Date endDate;
	String rentalReference;
	//variables omitted
	//methods:
	public boolean isAvailableForDate(Date date) //check if car is available on given date. details omitted.
	public void setRetentalInfo(String rentalRef, Date endDate) //set rental status, details omitted.
	//...other details omitted
}

Customer {
	private String id;
	private List<RentalRecord> records;
	//...other details omitted
}

RetentalRecord {
	private String id;
	private String carReg;
	private String customerId;
	private Date startDate;
	private Date endDate;
	//...other details omitted
}

//The DataService class is from an external jar and cannot be changed.
DataService {
	public Object load(String sqlString, Class type) throws SQLException;
	public Object saveToDatabase(Object data) throws SQLException;
	public  void delete(String id, Class type) throws SQLException;
}

//The main focus is on the class below
RentalService{
	private DataServicAce;
	//The primary method called by a rest servlet.
	public String rentCar(String customerName, String licenseNumber, String carRegistration, Date startDate, Date endDate, double rate) throws Exception {
		Customer customer= getCustomerInfo(customerName, licenseNumber); //uses DataServcie to retrieve custumer details from db. Details omitted
		Car car = getCarInfo(reg); //uses DataServcie to retrieve car details from db. details omitted here
		if (!car.isAvailable()) throws new CarUnavailabeException();
		String rentalId= generateRentalId(); //details omitted here.
		RetentalRecord record=createRentalRecord(...) //detils omitted;
		customer.addRentalRecord(record);
		car.setRentalInfo(rentalId, endDate) ;
		dataService.saveToDatabase(customer); //assume the rental record attached to a customer will also be persisted.
		dataService.saveToDatabase(car);
		return rentalId;
	}

	public void returnCar(String  rentalId) {
		Car car = getCar(rentalId) //
		car.setRentalInfo(null, null);
		dataService.saveToDatabase(car);
	 }
}

The method rentCar() is not thread safe as one thread could see a car is availabe for hire but before the booking finishes,
another thread could see the same car being availabe and booked it before the original thread. Thus the first thread could override the 2nd thread's booking.
He suggested that he could make synchronized both the methods: rentCar() and returnCar(). This would make it thread safe.

The problem then is that the rentCar() method would be called by many requests. Making it serial will hit the server performance hard,
and he knew he would have to make it multi-threaded, but struggled on how to do it.

Here is my solution, the idea is:
1) Use multiple threads in a thread pool to increase performance. Create hiring and releasing tasks as Callable.
2) To avoid the situation where a car may be booked by multiple threads, guard the booking code with a lock so booking on a car
can only be done by one thread.
3) Use a ConcurrencyHashMap to stores a lock for each Car identified by its registration key. Any thread wants to book a car will
have to acquire the lock for the car before a booking can be made. Given the ConcurrentHashMap is thread safe and locks on its segments,
it will enable many requests being run concurrently, thus improve performance.

The code is listed below and I'll appreciate comments on whether it works; how to safely remove the individual locks? Alternative solutions?

public class ConcurrentHireService {
    private DataService dataService;
    private ConcurrentHashMap<String, Object> lockMap = new ConcurrentHashMap<>();
    private ExecutorService exService = Executors.newFixedThreadPool(10); //thread number can be adjusted later.

    //called by rest servlet to make booking
    public String rentCar(String customerName, String licenseNumber, String carRegistration, Date startDate, Date endDate, double rate) throws CarUnavailableException {
        HireTask task = new HireTask(name, licenseNumber, reg, startDate, endDate, rate);
        String hireRef = (String) exService.submit(task).get(); //...exception details omitted for brevity.
        return hireRef;
    }

    public void markReturned(String hireReference) {
        exService.submit(new ReleaseTask(hireReference));
    }


    public Car getCarInfo(String rg) {
        //... return from database car details. details omitted.
    }


    public Customer getCustomerInfo(String name, String licenseNumber) {
        //... return from database customer details. details omitted.
    }

	//The class provides thread safe code for hiring
    private class HireTask implements Callable<HireTask> {
        private String customerName;
        private String licenseNumber;
        private String reg;
        private Date startDate;
        private Date endDate;
        private double rate;

        public HireTask(String customerName, String licenseNumber, String reg, Date startDate, Date endDate, double rate) {
            this.customerName = customerName;
            this.licenseNumber = licenseNumber;
            this.reg = reg;
            this.startDate = startDate;
            this.endDate = endDate;
            this.rate = rate;
        }

        @Override
        public Object call() throws Exception {
            String hireRef = UUID.randomUUID().toString(); //use as unique id
            try {
                Customer customer = getCustomerInfo(customerName, licenseNumber);
                Car car = getCarInfo(reg);
		//get a lock before operate on the car.
                Object newLock = new Object();
		//if there is no lock, then put one in for the particular car.
                Object storedLock= lockMap.putIfAbsent(reg, newLock);
                Object lockToUse = storedLock== null) ? newLock : storedLock
                synchronized (lockToUse) {
                    if (!car.isAvailableOnDate(startDate)) {
                        throw new CarUnavailableException("The car: " + car.getReg() + " is not available on date" + startDate);
                    }
                    RetentalRecord record = new HireRecord(car, client, Date.valueOf(start), days, rate, 1, hireRef);
                    client.addRentalRecord(record);
                    car.setRentalInfo(hireRef, endDate);
                    dataService.saveToDatabase(car);
                    dataService.saveToDatabase(customer);
                }
            } catch(SQLException ex){
                new RuntimeException("Cannot save rental record.");
            }
            return hireRef;
        }
    }

    private class ReleaseTask implements Callable<ReleaseTask> {
        String hireReference;
        public ReleaseTask(String hireReference) {
            this.hireReference=hireReference;
        }

        public ReleaseTask call() throws Exception {
            try {
                Car car = (Car) db.loadFromDb(connectionDetails, "select * from crs where hrrnm = " + hireReference, Car.class);
                Object newLock = new Object();
                String registrationNo=car.getReg();
                Object storedLock= lockMap.putIfAbsent(registrationNo, newLock);
                Object lockToUse = storedLock== null) ? newLock : storedLock;
                synchronized (lockToUse) {
                    car.release();
                    dataService.saveToDatabase(car);
                }
            } catch (SQLException e) {
                new RuntimeException("Problem encountered, unable to complete the return of the car.");
            }
            return null;
        }
    }
}

[Stephan van Hulst]

There's no point in using an executor service. Your requests are already handled concurrently by the application server, and adding more threads to the mix just increases overhead.

Honestly, the initial setup was doomed from the start. All objects should be immutable, and the service should let locking be handled by the database, through the use of transactions.

[Stephan van Hulst]

Welcome to CodeRanch!

[Ian Norris]

Hi Stephan,
Thank you for the reply.
I agree with you that in a real system, using database transaction would have been the more straight forward solution.
But given this was an interview question and the interviewers said that the DataService was from an external jar and that's the only thing one could use for interacting with the database and the DataService has only those 3 methods given, which apparently has no transaction facility.
Presumably, they want test a candidate's ability of developing concurrency  code? With all those constraints, will this solution work?

Ian

[Stephan van Hulst]

Well, I would probably first find out if I can abuse the loadFromDb() method to create transactions anyway, and then write a facade around that.

Assuming they want to test your concurrency chops though, having a map of locks seems like a decent first approach. However, as I've said before there is absolutely no point to having the bookings performed by a separate executor service (as a matter of fact, it's very bad practice to spawn threads in a web application).

You also shouldn't be using the synchronized statement, or Object for locks. Java introduced a high level concurrency library in SE5, I believe. You might want to take a look at java.util.concurrent.locks.ReadWriteLock for this one.

I'll look at this some more tomorrow, I'm on my phone right now.