Problem with Synchronized method using RMI

[Howard Kresin]

I have a basic data class Customer which has 4 fields, LastName, FirstName, UserID, and TransactionNo

My RMI setup is as follows:

Client takes LastName and FirstName as arguments and gets the UserID and TransactionNo programmatically

The Client looks like the following:
<After connecting to the server>

  customer.setFirstName(args[0]);
  customer.setLastName(args[1]);
  customer.setUserNo(userNumber);
  customer.setTransNo(transNo++);
// Print out a copy of all the data before interacting with the server
  out.println("Before :" + customer);
// Send the data to the server and get it right back
  customer = (Customer) rmi.SetGet(customer);
// Print out what I got back from the server to verify that it the same
  out.println("After : " + customer);

The ServerImpl can process the data from the Client and looks like:

public synchronized Customer SetGet(Customer customerData) throws RemoteException {
    //synchronized (this.customer) {
  setCustomer(customerData);
  return getCustomer();
}
 
public Customer getCustomer() throws RemoteException {
  return customer;
}
 
public void setCustomer(Customer customer) throws RemoteException {
  this.customer.setFirstName(customer.getFirstName());
  this.customer.setLastName(customer.getLastName());
  this.customer.setUserNo(customer.getUserNo());
  this.customer.setTransNo(customer.getTransNo());
}
 

The problem that I am having is that if I instance two Clients and make each Client instance loop around 1000 time, most of the time I will get what I expect:

Before : Client A�s data
After : Client A�s data

Every so often though, I will see:
Before : Client A�s data
After : Client B�s data

This happened whether I synchronized on the SetGet function or on the customer object. (The code shows it both ways)

More detailed logging of ServerImpl indicates that where the problems appear (I can tell by the TransNo) after entering the synchronized block or function, the block or function is interrupted by the other Client.

Any help would be appreciated.

[Erik Larson]

It took me a while to recreate your problem, but I finally did. The synchronization is not really the problem. I did tests and I did not see an instance in which two Threads had access to the Server.customer at one time. Where the problem comes in is that you are returning a reference to the Server.customer. This same reference is then locked by the next Thread that tries to access the GetSet method. The problem is that on the client, you use the same reference. Scenario:

1. Client 1 calls GetSet()
2. GetSet() returns with Server.customer reference, values set to Client 1
3. Context switch to Client 2.
4. Client 2 calls GetSet()
5. GetSet() returns with Server.customer reference, values set to Client 2
6. Context switch to Client 1.
7. Client 1 prints customer.toString() with values from Client 2 because customer in Client 1 holds the same reference, but with updated content.

My solution was to make Customer implement Cloneable and return a clone of Customer from getCustomer(). I'll put all my code in here...

Customer.java

  
import java.io.Serializable;
 
public class Customer implements Serializable, Cloneable {
String firstName;
String lastName;
int userNo;
int transactionNo;

public Customer() {
firstName = "";
lastName = "";
userNo = 0;
transactionNo = 0;
}
 
// Getters and Setters for each field omitted
 
public boolean equals(Object obj) {
Customer c = (Customer)obj;
return (c.firstName.equals(firstName) &&
c.lastName.equals(lastName) &&
c.userNo == this.userNo &&
c.transactionNo == this.transactionNo);
}
 
public int hashCode() {
return firstName.length() + lastName.length() + transactionNo;
}

public String toString() {
return firstName + " " + lastName + ": " + userNo + "/" + transactionNo; 
}
 
protected Object clone() {// throws CloneNotSupportedException {
Customer clone = new Customer();
clone.firstName = firstName;
clone.lastName = lastName;
clone.userNo = userNo;
clone.transactionNo = transactionNo;
return clone;
}
}

Server.java

  
import java.rmi.Remote;
import java.rmi.RemoteException;
 
public interface Server extends Remote {
public Customer SetGet(Customer customerData) throws RemoteException;
}

ServerImple.java

  
import java.rmi.Naming;
import java.rmi.RemoteException;
import java.rmi.registry.LocateRegistry;
import java.rmi.server.UnicastRemoteObject;
 
public class ServerImple extends UnicastRemoteObject implements Server {
 
Customer customer;

public ServerImple() throws RemoteException {
customer = new Customer();
}
 
public synchronized Customer SetGet(Customer customerData) throws RemoteException {
//synchronized (this.customer) {
setCustomer(customerData);
return getCustomer();
//}
}
 
public Customer getCustomer() {
  return (Customer)(customer.clone());
}
 
public void setCustomer(Customer customer) {
  this.customer.setFirstName(customer.getFirstName());
  this.customer.setLastName(customer.getLastName());
  this.customer.setUserNo(customer.getUserNo());
  this.customer.setTransNo(customer.getTransNo());
}

public static void main(String[] args) {
int port = 1099;
try {
LocateRegistry.createRegistry(port);
Naming.rebind("//localhost:" + port + "/" + "CustomerTest", new ServerImple());
System.out.println("Server Running");
} catch (Exception e) {
e.printStackTrace();
}
while(true);
}
}

ThreadedCustomer.java

import java.rmi.Naming;
import java.rmi.RemoteException;
 
class ThreadedCustomer implements Runnable {
 
public Server rmi;
 
int user;
 
public ThreadedCustomer(int user) {
this.user = user;
try {
rmi = (Server) Naming.lookup("//localhost:1099/CustomerTest");
} catch (Exception e) {
e.printStackTrace();
}
}
 
public void run() {
Customer customer = new Customer();
 
for (int transNo = 0; transNo < 500; transNo++) {
customer.setFirstName("F" + user);
customer.setLastName("L" + user);
customer.setUserNo(user);
customer.setTransNo(transNo);
 
try {
Customer returnCustomer = (Customer) rmi.SetGet(customer);
if (!customer.equals(returnCustomer)) {
System.out.println(customer + "|" + returnCustomer);
throw new RuntimeException();
}
} catch (RemoteException e) {
e.printStackTrace();
}
}
}
 
public static void main(String[] args) {
for (int i = 1; i < 10; i++)
new Thread(new ThreadedCustomer(i), "Thread-" + i).start();
}
}

Hope this helps!