Search code examples
javanetwork-programmingrunnable

java singleton network implementing runnable


I am programming a Network singleton class and it needs to run in a thread, so far no problem, however I cannot get it work sadly.

Network class:

public class Network extends Thread {    
    private static Network cachedInstance = new Network();

    private PrintWriter out;
    private BufferedReader in;

    private Network() {        
    }

    private void init() {
        try {
            Socket clientSocket = new Socket(Config.HOST_NAME, Config.HOST_PORT);
            out = new PrintWriter(clientSocket.getOutputStream(), true);
            in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));

            String fromServer;
            while ((fromServer = in.readLine()) != null) {
                System.out.println("Server: " + fromServer);
            }

        } catch (IOException ex) {
            Logger.getLogger(Controller.class.getName()).log(Level.SEVERE, null, ex);
        }
    }

    public static Network getInstance() {
        return cachedInstance;
    }

    public void send(final String string) {
        out.println(string);
    }

    @Override
    public void run() {
        init();
    }
}

Part of Controller class:

public void clientTest() {
    int random = new Random().nextInt(1000);
    Network.getInstance().start();
    Network.getInstance().send(random + "");
}

The error I am getting:

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
at network.Network.send(Network.java:52)
at controller.Controller.clientTest(Controller.java:126)

So it is looking like the single Network instance is not instantiated properly, which theoretically shouldn't be possible.

A second question I have is if I can avoid using this:

Network.getInstance().start();

In other words I would want to ensure that only one thread (Network class) is created and that it is always running by default when the classes are initialized. It is not bad in the current way but I just figured it would be nicer.

For people wondering why I use this approach: Basically I only want to use Network.send() to send to a fixed server. That server can of course send stuff back aswell, but upon that Network needs to react and invoke methods from the Controller at some point.

Regards.

EDIT: Proposed Solution, based on reactions

Network.class:

public class Network implements Runnable {    
    private static final Network cachedInstance;
    static {
        Network tempInstance = null;
        try {
            tempInstance = new Network(Config.HOST_NAME, Config.HOST_PORT);
        } catch (IOException ex) {
            Logger.getLogger(Network.class.getName()).log(Level.SEVERE, null, ex);
        } finally {
            cachedInstance = tempInstance;
        }
    }

    private final Socket clientSocket;
    private final PrintWriter out;
    private final BufferedReader in;

    private Network(final String hostname, final int port) throws IOException {
        clientSocket = new Socket(hostname, port);
        out = new PrintWriter(clientSocket.getOutputStream(), true);        
        in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
    }

    public static Network getInstance() {
        return cachedInstance;
    }

    @Override
    public void run() {
        try {
            String fromServer;
            while ((fromServer = in.readLine()) != null) {
                System.out.println("Server: " + fromServer);
            }
        } catch (IOException ex) {
            Logger.getLogger(Network.class.getName()).log(Level.SEVERE, null, ex);
        }
    }

    public void send(final String string) {
        out.println(string);
    }

    public void close() {
        try {
            in.close();
            out.close();
            clientSocket.close();
        } catch (IOException ex) {
            Logger.getLogger(Network.class.getName()).log(Level.SEVERE, null, ex);
        }
    }
}

Calling code:

public void clientTest() {
    int random = new Random().nextInt(1000);
    Network network = Network.getInstance();
    new Thread(network).start();
    network.send(random + "");
    network.close();
}

This is just for testing, in reality the connection needs to stay open until the user closes the program.


Solution

  • A simple solution to the problem is to have the connect established before you start the thread so you don't have this race condition to worry about.

    public class Network implements Runnable, Closeable {    
        private final Socket clientSocket;
        private final PrintWriter out;
        private final BufferedReader in;
        private volatile boolean closed = false;
    
        public Network(String hostname, int port) throws IOException {        
            clientSocket = new Socket(hostname, port);
            out = new PrintWriter(clientSocket.getOutputStream(), true);
            in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
        }
    
        public void run() {
            try {
                for(String fromServer; (fromServer = in.readLine()) != null;) 
                    System.out.println("Server: " + fromServer);
            } catch (IOException ex) {
                if (!closed)
                    Logger.getLogger(Controller.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
    
        public void send(String line) {
            out.println(line);
        }
    
        public void close() {
            closed = true;
            try { clientSocket.close(); } catch (IOException ignored) { }
        }
    }
    

    For testing

    @Test
    public void testClient() {
        Network network = new Network(Config.HOSTNAME, Config.PORT)
        new Thread(network).start();
    
        int random = new Random().nextInt(1000);
        network.send(random + "");
        network.close();
    }
    

    Note: using stateful singleton classes make unit testing very difficult because you have to reset the singleton back to it's initial state or one test can impact another and the order you run can make a difference. In the test above, it is self contained, and no other test is harmed. ;)