Singleton-ish RMI Server

Question

In this program there is a single remote server which performs miscellaneous work. Because there is only one server, it seems like a singleton pattern could fit well.

The problem that a distributed program introduces is that every client will have its own version of a static variable. Therefore I have defined a basic ComputeInterface which is implemented by two classes. ComputeServer is the object which performs the work asked for by clients. ComputeService is a class which performs an RMI lookup to the ComputeServer and acts like singleton server in the client program.

The location information for the Server is maintained in a Configuration class:

public class Configuration {
    public static final int COMPUTE_REMOTE_PORT = 8900;
    public static final String COMPUTE_REMOTE_ID = "COMPUTE_REMOTE_ID";
    public static final String COMPUTE_REMOTE_HOST = "localhost";
}

A ComputeTask implements Serializable and is performed by the ComputeInterface which implements Remote. The ComputeInterface contains an isConnected method which is used for determining if there is connectivity to the server from the service:

public interface ComputeTask extends Serializable{
    void doWork();
}

public interface ComputeInterface extends Remote {
    ComputeTask compute(ComputeTask task) throws RemoteException;
    boolean isConnected() throws RemoteException;
}

This is the implementation for the server:

final class ComputeServer extends UnicastRemoteObject implements ComputeInterface {
    public static void main(String[] args) throws RemoteException, AlreadyBoundException {
        ComputeServer computeServer = new ComputeServer();
        Registry registry = LocateRegistry.createRegistry(Configuration.COMPUTE_REMOTE_PORT);
        registry.bind(Configuration.COMPUTE_REMOTE_ID, computeServer);
        System.out.println("Compute Server Running");
    }

    private ComputeServer() throws RemoteException {
        super();
    }

    @Override
    public synchronized ComputeTask compute(ComputeTask task) throws RemoteException{
        task.doWork();
        return task;
    }

    @Override
    public synchronized boolean isConnected() throws RemoteException{
        return true;
    }
}

And here is the Service. If the server has not yet been connected, the service will attempt to do a registry lookup on the server. If the registry lookup up was successful (or has already been performed), it will check the server connectivity before performing any tasks.

final public class ComputeService implements ComputeInterface{

    public static final ComputeInterface service = new ComputeService();
    private static ComputeInterface computeServer;

    private ComputeService(){}

    @Override
    public synchronized boolean isConnected() throws RemoteException{
        return ComputeService.connected();
    }

    private static boolean connected(){
        if (computeServer == null) {
            try{
                Registry reg = LocateRegistry.getRegistry(Configuration.COMPUTE_REMOTE_HOST, Configuration.COMPUTE_REMOTE_PORT);
                computeServer = (ComputeInterface) reg.lookup(Configuration.COMPUTE_REMOTE_ID);
                System.out.println("New Connection to Compute Server");
            }
            catch (RemoteException | NotBoundException e){
                System.err.print(e);
                computeServer = null;
                return false;
            }
        }
        try{
            return computeServer.isConnected();
        }
        catch (RemoteException e){
            System.err.print(e);
            computeServer = null;
        }
        return false;
    }

    @Override
    public synchronized ComputeTask compute(ComputeTask task) throws RemoteException{
        if(!ComputeService.connected()) return task;
        return computeServer.compute(task);
    }
}

Here is an example of how a client can use the service to perform work on the server:

CustomComputeTask task = new CustomComputeTask();
task = ComputeService.service.compute(task);

Answer 1

In this program there is a single remote server which performs miscellaneous work. Because there is only one server, it seems like a singleton pattern could fit well.

Let us see the definition from Wikipedia:

In software engineering, the singleton pattern is a design pattern that restricts the Instantiation of a class to one object. This is useful when exactly one object is needed to coordinate actions across the system. The concept is sometimes generalized to systems that operate more efficiently when only one object exists, or that restrict the instantiation to a certain number of objects. The term comes from the mathematical concept of a singleton.

So basically you are on the right track but what can't a simple private final object accomplish what the singleton can ?

Furthermore, which disadvantages has using a singleton ?

Quoting from here

Singletons are essentially static classes, relying on one or more static methods and properties. All things static present real, tangible problems when you try to do Unit Testing because they represent dead ends in your code that cannot be mocked or stubbed. As a result, when you test a class that relies on a Singleton (or any other static method or class) you are not only testing that class but also the static method or class.

and here

It is because of this specific design choice that the pattern introduces several potential long-term problems:

• Inability to use abstract or interface classes;
• Inability to subclass;
• High coupling across the application (difficult to modify);
• Difficult to test (can't fake/mock in unit tests);
• Difficult to parallelize in the case of mutable state (requires extensive locking);
  • and so on.

None of these symptoms are actually endemic to single instances, just the Singleton pattern.

What can you do instead? Simply don't use the Singleton pattern.

So basically this boils down, if you really need to use it then use it, but make sure you understand the possible pitfalls and possible upcoming problems.

That being said, let me review some of your code.

---

public class Configuration {
    public static final int COMPUTE_REMOTE_PORT = 8900;
    public static final String COMPUTE_REMOTE_ID = "COMPUTE_REMOTE_ID";
    public static final String COMPUTE_REMOTE_HOST = "localhost";
}  

What will happen if the port or the remote host needs to be changed ? You will need to recompile this class because all members are static and can't be changed. How about testability of the server /client code ?

Using this setup also restricts the server to be on the same computer than the client(s).

---

Based on how you have implemented the ComputeService and the ComputeInterface I would like to suggest that you add another method to the ComputeTask interface returning if the task really has been processed. Right now if the ComputeService can't connect to the server the returned ComputeTask object doesn't tell the client that it hadn't been processed.

The private static boolean connected() method doesn't throw a RemoteException and therefor either the isConnected() method of the ComputeInterface shouldn't have a throws RemoteException or better, let the connected() really throw that exception.

Right now if this method returns false the client doesn't know what the problem is.

In addition, I would name the connected() method just isConnectedToServer() or something along this line. But nevertheless this method is doing to much because it is doing a registry lookup so to say it is connecting to the server, but is read like a simple "property getter" .

I would change isConnected() method like so

@Override
public synchronized boolean isConnected() throws RemoteException{
    if (computeServer == null)
    {
        return false;
    }
    return computeServer.isConnected();
}

Then I would replace the connected() method by a, to the ComputeInterface added, boolean connect() method which does exactly what the name implies like so

@Override
public synchronized boolean connect() throws RemoteException{
    if (computeServer == null) {
        try{
            Registry reg = LocateRegistry.getRegistry(Configuration.COMPUTE_REMOTE_HOST, Configuration.COMPUTE_REMOTE_PORT);
            computeServer = (ComputeInterface) reg.lookup(Configuration.COMPUTE_REMOTE_ID);
            System.out.println("New Connection to Compute Server");
        }
        catch (RemoteException | NotBoundException e){
            System.err.print(e);
            computeServer = null;
            throw e;
        }
    }
    return true;
}

and if you want to add some kind of authentication, I would add a boolean connect(someparameters) method to the ComputeInterface which then is called instead of just returning true.

---

The most recent question for me is if you really should use RMI for this application. RMI has some disadvantages like

• it is using blocking IO
• if you want to use callbacks, you will likely come into trouble if a router or firewall is set up
• and some more

IMHO a much better alternative would be SIMON ( Simple Invocation of Methods Over Network), which is actively developed and maintained. I use this myself for a multi-server/multi-client system and don't regret its use.

Answer 2

A couple nitpicks:

1. When using braces, try to include a space between the identifier before the brace and the brace itself. This should also be applied to parenthesis on if, for, foreach, etc. Basically, parenthesis that are not for a method call:

final public class ComputeService implements ComputeInterface{

Should be:

final public class ComputeService implements ComputeInterface {

2. When casting, don't add whitespace between the cast and the variable being casted:

(ComputeInterface) reg.lookup(Configuration.COMPUTE_REMOTE_ID);

Should be:

(ComputeInterface)reg.lookup(Configuration.COMPUTE_REMOTE_ID);

3. Never, never, never, never, never include if statements (without braces) and their action on the same line:

if(!ComputeService.connected()) return task;

Should be:

if (!ComputeService.connected())
    return task;

Or, the preferred (with braces):

if (!ComputeService.connected())
{
    return task;
}

---

On to some substance

This:

public class Configuration {
    public static final int COMPUTE_REMOTE_PORT = 8900;
    public static final String COMPUTE_REMOTE_ID = "COMPUTE_REMOTE_ID";
    public static final String COMPUTE_REMOTE_HOST = "localhost";
}

Is generally bad. What happens if, for some reason, you have to change the COMPUTE_REMOTE_HOST on only one instance? You have to alter the Configuration class, recompile, and remember to change it back for something different. Make Configuration an instance class, instead. Then, pass that instance to each class that needs it. (Yes, it's more work, but it's also more extensible, more maintainable, and follows more best-practices.)

Yes, you want the singleton design, but you don't have to have everything as a singleton to satisfy the idea of the design. Some things, are just bad as singleton's.

---

It also would not hurt to add some comments, explaining why you do some things the way you do. Comments, in code, are generally helpful to remind yourself in the future of why something is written in this form. That way, when a more experienced you (or other developer) comes along, you/he/she can say, "Oh! That's why it is written like this." Rather than, "this is not the greatest, I should change it." Then things break.