Alter centralized logging code to be thread safe

Question

I am using this code for receiving log messages from my clients. I receive more than 1000 connections per minute and I want to increase my log handling, which I have done with java threading.

What happens if it receive multiple client connections? Is this thread safe?

public class CentralizedLogging implements Runnable {

    /**
    * Field logDir - Where the plugin logs can be stored .
    */
    public static String logDir;
    /**
    * Field server - Server Socket .
    */
    ServerSocket server = null;
    /**
    * Field LOGGER.
    */
    public static final Logger LOGGER = Logger.getLogger(CentralizedLogging.class.getName());

    /**
    * @param port - Port which the server is binds .
    * @param logDir String
    */
    public CentralizedLogging(String logDir, int port) {
        try {
            this.logDir = logDir;
            server = new ServerSocket(port);
        } catch (IOException e) {
            LOGGER.log(Level.SEVERE, "It may be due to given port already binds with another process , reason {0}", new Object[]{e});
        }
    }

    /**
    * Extension point for central log server . To start receiving connections from remote end .
    */
    public void run() {
        while (true) {
            try {
                new Thread(new LogWriter(server.accept())).start();
            } catch (Exception e) {
                LogWriter.log("Interrupted exception " + e.getMessage());
            }
        }
    }

    /**
    * args[0] - logging location . args[1] - which port the server can start. It must be a integer.
    *
    * @param args
    */
    public static void main(String[] args) {
        try {
            CentralizedLogging logServer = new CentralizedLogging(args[0], Integer.parseInt(args[1]));
            new Thread(logServer).start();
        } catch (Exception e) {
            LOGGER.log(Level.SEVERE, "Unable to start log server from this location : {0} , port : {1} , This may be due to given port is not a number or this port is not free  exception trace {2}", new Object[]{args[0], args[1], e});
        }
    }
}

/**
* Used for writing client packets into logs Dir .
*/
class LogWriter implements Runnable {

    /**
    * Field client - Socket Client .
    */
    Socket client;

    /**
    * Constructor for LogWriter.
    *
    * @param client Socket
    * @throws IOException
    */
    public LogWriter(Socket client) {
        this.client = client;
    }

    public void run() {
        write();
        try {
            this.client.close();
        } catch (IOException io) {
            System.out.println("Error while closing connection , reason " + io);
        }
    }

    /**
    * Method write.
    */
    public void write() {
        try {
            String date = new SimpleDateFormat("yyyy_MM_dd").format(new Date());
            File file = new File(CentralizedLogging.logDir + client.getInetAddress().getHostName() + "_"
                    + date + ".log");
            write(client.getInputStream(), file);
        } catch (Exception e) {
            log("Error in writing logs :: Host Name " + client.getInetAddress().getHostName()
                    + " , Occured Time " + System.currentTimeMillis() + ", Reason " + e.getMessage() + "\n\n");
        }
    }

    /**
    * Method write.
    *
    * @param in InputStream
    * @param file File
    * @throws Exception
    */
    public synchronized static void write(InputStream in, File file) throws Exception {
        RandomAccessFile writer = new RandomAccessFile(file, "rw");
        writer.seek(file.length()); //append the file content to the existing file or creates a new one .
        writer.write(read(in));
        writer.close();
    }

    /**
    * This method is used for monitoring errors that will be occured on writing plugin logs .
    *
    * @param msg
    */
    public static void log(String msg) {
        File file = new File(CentralizedLogging.logDir + "plugin_error_" + new SimpleDateFormat("yyyy_MM_dd").
                format(new Date()) + ".log");
        try {
            write(new ByteArrayInputStream(msg.getBytes()), file);
        } catch (Exception e) {
        }
    }

    /**
    * Method read.
    *
    * @param in InputStream
    * @return byte[]
    * @throws IOException
    */
    public static byte[] read(InputStream in) throws IOException {
        ByteArrayOutputStream out = new ByteArrayOutputStream();
        int read = -1;
        byte[] buffer = new byte[1024];
        while ((read = in.read(buffer)) > -1) {
            out.write(buffer, 0, read);
        }
        return out.toByteArray();
    }

    /**
    * Method close.
    *
    * @param stream Object
    */
    public static void close(Object stream) {
        try {
            if (stream instanceof Writer) {
                ((Writer) stream).close();
            } else if (stream instanceof Reader) {
                ((Reader) stream).close();
            }
        } catch (Exception e) {
        }
    }
}

Answer 1

From what i see, it looks like you got the threads started correctly. You said in your question that you are making 1000 connections per minute or something to that sort, which is 1000 threads every minute. If you want to make your program more memory efficient, you might want to set the threads to null when you're done using them.

When i program, i defiantly try to stay away from calling threads on the fly, especially if I'm programming a device that has limited memory resources. Here is some code i came up with, that is similar to what i use:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.ArrayList;
import java.util.Iterator;

public class GameDev1 
{
    public GameDev1()
    {
        handlers = new ArrayList<ConnectionHandler>();
        try
        {
            server = new ServerSocket(8080); // use whatever port you need
        }catch(Exception e){e.printStackTrace();}
    }

    public void run()
    {
        //This is the run() method for your server
        int connections = 0;

        while(true)
        {
            try {
                handlers.add(new ConnectionHandler(server.accept())); // adds the new ConnectionHandler to the list
                connections++;

                // this will clear unused memory every time 1000 people connect to your sever, you can set this to more
                // or less, whatever you want
                if(connections == 1000)
                {
                    cleanHandlers();
                    connections = 0;
                }
            }catch(Exception e){e.printStackTrace();}
        }
    }

    public void cleanHandlers()
    {
        Iterator<ConnectionHandler> it = handlers.iterator();// this is used to sort through the list

        while(it.hasNext())
        {
            ConnectionHandler next = it.next();
            if(next.finished)
            {
                // when a ConnectionsHandler is done, it sets finished to true, which means clean should be called
                next.clean();
                it.remove();
            }

            next = null;
        }
    }

    public ArrayList<ConnectionHandler> handlers; // this is a list of all of the ConnectionHandlers that exist in memory.
    public ServerSocket server; // your server socket.

    public class ConnectionHandler implements Runnable
    {
        public ConnectionHandler(Socket socket)
        {
            this.socket = socket;
            finished = false;

            thread = new Thread(this);
            thread.start();
        }

        public void run()
        {
            // Do your run method here, this is where you collect the log information from the client
            //...

            finished = true;
        }

        public void clean()
        {
            // this cleans up anything that the class is no longer using

            if(finished)
            {
                try
                {
                    socket.close();
                }catch(Exception e){e.printStackTrace();}
                socket = null;

                if(thread != null)
                    thread.interrupt();
                thread = null;
            }
        }

        public Socket socket;

        public Thread thread;
        public boolean finished;
    }
}

When your computer runs low on memory, Java will tell the garbage collector to run, which should free up some memory. I never like to leave threads "dangling." I always make sure to set a thread equal to null when its done with its cycle, especially when I'm creating a bunch of them. Your code looks good from what i could see, this is just a suggestion of course so i hope it helped!