HTTP server and multi-threading optimization

Question

I wrote an HTTP server for the management of scores for users and levels.

It can return the highest score per level. It has a simple login with session-key.

What do you think could be improved in this code? What's wrong? And particularly, are there any multi-threading issues that I didn't take into account?

//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: demonstration of an http server
//
//*********************************************************

package org.bombo.server;

import java.io.IOException;
import java.net.InetSocketAddress;
import java.util.concurrent.Executors;

import com.sun.net.httpserver.HttpContext;
import com.sun.net.httpserver.HttpServer;

public class Main {

    private static int port = 8009;

    public static void main(String[] args) throws IOException {

        // Create an http server
        HttpServer server = HttpServer.create(new InetSocketAddress(port), 0);

        // Create a context
        HttpContext context = server.createContext("/", new RequestsHandler());

        // Add a filter
        context.getFilters().add(new ParamsFilter());

        // Set an Executor for the multi-threading
        server.setExecutor(Executors.newCachedThreadPool());

        // Start the server
        server.start();

        System.out.println("The server is started!");
    }  
}

//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: filter to set the request parameters
//
//*********************************************************

package org.bombo.server;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.UnsupportedEncodingException;
import java.net.URI;
import java.util.HashMap;
import java.util.Map;

import org.bombo.util.MiscUtils;

import com.sun.net.httpserver.*;

public class ParamsFilter extends Filter {

    MiscUtils utils;

    public ParamsFilter() {
        utils = new MiscUtils();
    }

    /*
     * Usage: filter the request before to be handled and prepare the 
     *     parameters
     * 
     * Input:
     *    exchange = request/response object
     *    
     */
    @Override
    public void doFilter(HttpExchange exchange, Chain chain)
        throws IOException {
        parseGetParameters(exchange);
        parsePostParameters(exchange);
        parseUrlEncodedParameters(exchange);
        chain.doFilter(exchange);
    }    

    /*
     * Usage: retrieve the GET parameters
     * 
     * Input:
     *    exchange = request/response object
     *    
     */
    private void parseGetParameters(HttpExchange exchange)
        throws UnsupportedEncodingException {

        Map<String, Object> parameters = new HashMap<String, Object>();
        URI requestedUri = exchange.getRequestURI();
        String query = requestedUri.getRawQuery();        
        utils.parseQuery(query, parameters);
        exchange.setAttribute("parameters", parameters);
    }

    /*
     * Usage: retrieve the POST parameters
     * 
     * Input:
     *    exchange = request/response object
     *    
     */
    private void parsePostParameters(HttpExchange exchange)
        throws IOException {

        if ("post".equalsIgnoreCase(exchange.getRequestMethod())) {
            @SuppressWarnings("unchecked")
            Map<String, Object> parameters =
                (Map<String, Object>)exchange.getAttribute("parameters");
            InputStreamReader isr =
                new InputStreamReader(exchange.getRequestBody(),"utf-8");
            BufferedReader br = new BufferedReader(isr);
            String query = br.readLine();
            utils.parseQuery(query, parameters);
        }
    }

    /*
     * Usage: retrieve the URL encoded parameters
     * 
     * Input:
     *    exchange = request/response object
     *    
     */
    private void parseUrlEncodedParameters(HttpExchange exchange)
        throws UnsupportedEncodingException {

        @SuppressWarnings("unchecked")
        Map<String, Object> parameters =
                (Map<String, Object>)exchange.getAttribute("parameters");

        String uri = exchange.getRequestURI().toString();
        String[] tokens = uri.split("[/?=]");

        if(tokens.length > 2) {
            if(tokens[2].equals("score") || tokens[2].equals("highscorelist")) {
                parameters.put("levelid", tokens[1]);
                parameters.put("request",tokens[2]);
            } else if(tokens[2].equals("login")) {
                parameters.put("userid", tokens[1]);
                parameters.put("request",tokens[2]);
            }
            else {
                parameters.put("request","not supported");
            }
        }
        else {
            parameters.put("request","not supported");
        }
    }

    @Override
    public String description() {
        return "Class to retrieve Get/Post parameters";
    }
}


//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: handler of a request (every handler instance is a thread)
//
//*********************************************************

package org.bombo.server;

import java.io.IOException;
import java.io.OutputStream;
import java.util.Map;
import com.sun.net.httpserver.*;
import org.bombo.data.*;

class RequestsHandler implements HttpHandler {


    /*
     * Usage: handle a client request
     * 
     * Input:
     *    exchange = request/response object
     *    
     */
    public void handle(HttpExchange exchange) throws IOException {

        String response = null;
        int statusCode = 200;

        try {
            @SuppressWarnings("unchecked")
            Map<String, Object> params = (Map<String, Object>)exchange.getAttribute("parameters");

            if (params.get("request").equals("login")) {

                // The user is logging-in, asking for a session key
                System.out.println("New login request received for the user" +
                        (String)params.get("userid"));

                response = SessionSingleton.getInstance().getSessionKey(
                        Integer.parseInt((String)params.get("userid")));

                if(response == null) statusCode = 500; // Server error
            }
            else if (params.get("request").equals("score")) {

                // A new store has been received to be stored
                System.out.println("New request received to save the score" +
                        (String)params.get("score"));

                int userId = SessionSingleton.getInstance().validateSessionKey(
                        (String)params.get("sessionkey"));

                if (userId  == -1) 
                    statusCode = 401; // Unhautorized user
                else if(ScoreSingleton.getInstance().insertScore(
                        userId, Integer.parseInt((String)params.get("levelid")),
                        Integer.parseInt((String)params.get("score"))) == -1)
                    statusCode = 500; // Server error
            }
            else if (params.get("request").equals("highscorelist")) {

                // A list of the best scores has been requested
                System.out.println("Received a new request for the highest scores" +
                        "the level " + (String)params.get("levelid"));

                response = ScoreSingleton.getInstance().getHighestScores(
                        Integer.parseInt((String)params.get("levelid")));

                // This is a header to permit the download of the csv
                Headers headers = exchange.getResponseHeaders();
                headers.add("Content-Type", "text/csv");
                headers.add("Content-Disposition", "attachment;filename=myfilename.csv");
            }
            else {
                response = "Method not implemented";
                System.out.println(response);
                statusCode = 400; // Request type not implemented
            }
        }
        catch (NumberFormatException exception) {
            statusCode = 400;
            response = "Wrong number format";
            System.out.println(response);
        }
        catch (Exception exception) {
            statusCode = 400;
            response = exception.getMessage().toString();
            System.out.println(response);
        }

        // Send the header response
        if (response != null)
            exchange.sendResponseHeaders(statusCode, response.length());
        else 
            exchange.sendResponseHeaders(statusCode, 0);

        // Send the body response
        OutputStream os = exchange.getResponseBody();
        os.write(response.toString().getBytes());
        os.close();
    }
}


//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: contain common math functionalities
//
//*********************************************************

package org.bombo.util;

import java.math.BigInteger;
import java.security.SecureRandom;

public class MathUtil {

    /*
     * SecureRandom guarantee secure random strings, very hard to hack, but 
     * the initialization is expensive and for this reason we are making
     * it static.
     */
    static private SecureRandom random = new SecureRandom();

    /*
     * Usage: create a new session key
     *    
     * Returns:
     *    Session key
     */
    public String createSessionKey() {
        return new BigInteger(130, random).toString(32);
    }

    /*
     * Usage: chek if a string is a positive integer and convert it
     *    
     * Input:
     *    str = string containing a number
     *    
     * Returns:
     *    -1 The string doesn't contain a valid positive integer
     *    n  The converted  number
     */
    public int getPositiveInt(String str) {

        int retInt = -1;

        try {
            retInt = Integer.parseInt(str);

            if (retInt < 0) {
                // Non positive integer
                return -1;
            }
        }
        catch(NumberFormatException nfe) {
            // Exception because the input is not a number
            System.out.println("Error: " + nfe.getMessage());
            return -1;
        }

        return retInt;
    }
}

//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: contain common util functionalities
//
//*********************************************************

package org.bombo.util;

import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.util.Map;

public class MiscUtils {
    /*
     * Usage: parse a query string in the format key=value&key=value and
     *      retrieve the values
     * 
     * Input:
     *    query = query string
     *    parameters (out) = list of key-value parameters
     *    
     */
    public void parseQuery(String query, Map<String, Object> parameters)
        throws UnsupportedEncodingException {

        if (query != null) {
            String pairs[] = query.split("[&]");

            for (String pair : pairs) {
                String param[] = pair.split("[=]");

                String key = null;
                String value = null;
                if (param.length > 0) {
                    // Retrieve the key
                    key = URLDecoder.decode(param[0],
                        System.getProperty("file.encoding"));
                }

                if (param.length > 1) {
                    // Retrieve the value
                    value = URLDecoder.decode(param[1],
                            System.getProperty("file.encoding"));
                }

                parameters.put(key.toLowerCase(), value);
            }
        }
    }
}


//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: Comparator implementation to order a score list
//
//*********************************************************

package org.bombo.util;

import java.util.Comparator;
import java.util.Map;

public class ScoreComparator implements Comparator<Integer> {

    Map<Integer, Integer> base;

    public ScoreComparator(Map<Integer, Integer> base) {
        this.base = base;
    }

    public int compare(Integer a, Integer b) {
        return -(base.get(a).compareTo(base.get(b)));
    }
}


//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: Store and manage scores by level and user
//
//*********************************************************

package org.bombo.data;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import java.util.concurrent.ConcurrentHashMap;
import org.bombo.util.ScoreComparator;;

public class ScoreSingleton
{
    static private ScoreSingleton singletonInstance;
    volatile private Map<Integer,Map<Integer,List<Integer>>> users;
    final int numHighestScores;

    private ScoreSingleton() 
    {
        // Thread safe data structure to store user->Level->Score informations
        users = new ConcurrentHashMap<Integer,Map<Integer,List<Integer>>>();
        numHighestScores = 15; // Max num of high scores to be returned
    }

    /*
     * Usage: return an instance of the singleton
     *    
     * Returns:
     *    Singleton instance
     */
    static public ScoreSingleton getInstance()
    {
        if ( singletonInstance == null )
            singletonInstance = new ScoreSingleton();

        return singletonInstance;
    }

    /*
     * Usage: permit to insert a score for a user, in a specific level
     * 
     * Input:
     *    user = user id
     *    level = level id
     *    score = last score
     *    
     * Returns:
     *    -1 Error
     *    0  Score added
     */
    public int insertScore(int user, int level, int score) {

        List<Integer> addScore = null;

        if (user < 0 || level < 0 || score < 0) {
            // The parameters are not valid
            return -1;
        }

        synchronized(users) { 

            // this block is synchronized because this is not an atomic operation

            if (users.containsKey(user)) {

                // Existing user

                Map<Integer,List<Integer>> storedUserLevels = users.get(user);

                if (storedUserLevels.containsKey(level)) {

                    // Existing level

                    List<Integer> storedScores =  storedUserLevels.get(level);

                    storedScores.add(score);
                }
                else {

                    // New level for existing user

                    addScore = Collections.synchronizedList(
                            new ArrayList<Integer>());

                    addScore.add(score);
                    storedUserLevels.put(level, addScore);
                    users.put(user, storedUserLevels);
                }
            } else {

                // New user     

                Map<Integer,List<Integer>> addLevel = 
                        new ConcurrentHashMap<Integer,List<Integer>>();
                addScore = Collections.synchronizedList(new ArrayList<Integer>());

                addScore.add(score);
                addLevel.put(level, addScore);
                users.put(user, addLevel);
            }
        }

        return 0;
    }

    /*
     * Usage: Get the n highest scores for a specific level (max 1 result per user)
     * 
     * Input:
     *    level = level id
     *    
     * Returns:
     *    null No score found
     *    String containing the list of scores
     */ 
    public String getHighestScores(int level) {

        Iterator<Integer> userIter = null;
        Map<Integer,Integer> scoreResults = new TreeMap<Integer,Integer>();
        int user = 0;
        Integer userMaxScore = 0;

        synchronized(users) {
            // this block is synchronized because of the iterator

            userIter = users.keySet().iterator();

            while(userIter.hasNext()) { // Search the highest score for each user

                user = userIter.next().intValue();

                if (users.get(user) != null &&
                    users.get(user).get(level) != null &&
                    users.get(user).get(level).size() > 0) {

                    // I enter here if there is a score for this user at the requested level

                    userMaxScore = Collections.max(users.get(user).get(level));

                    scoreResults.put(user,userMaxScore); // Get only the highest score
                }
            }
        }

        return retrieveBestScores(scoreResults);
    }

    /*
     * Usage: Order a list with the best scores and prepare a CSV string
     * 
     * Input:
     *    scores = list of the best scores (not ordered)
     *    
     * Returns:
     *    null No scores
     *    String containing the best scores (in inverse order) 
     *        (format: user=score\r\nuser=score...)
     */
    private String retrieveBestScores(Map<Integer,Integer> scores) {

        int user = 0;
        int contRes = 0;
        String resScores = null;

        // Map to order the scores by value (in desc order)
        TreeMap<Integer,Integer> sortedScores = 
                new TreeMap<Integer,Integer>(new ScoreComparator(scores));

        sortedScores.putAll(scores);

        Iterator<Integer> scoreIter = sortedScores.keySet().iterator();

        // I read all the scores starting from the highest
        while(scoreIter.hasNext()) {

            user = scoreIter.next();

            if (resScores == null)
                resScores = user + "=" + sortedScores.get(user).intValue() + "\r\n";
            else
                resScores += user + "=" + sortedScores.get(user).intValue() + "\r\n";

            if (++contRes >= numHighestScores) break; // Once got the max num of scores, exits
        }

        return resScores;
    }
}


//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: java bean to store info related to a single session
//
//*********************************************************

package org.bombo.data;

public class Session {

    long storedTime; // The time this session has been created
    int user; // User associated to this session

    public Session(long storedTime, int user) {

        this.storedTime = storedTime;
        this.user = user;
    }

    public long getStoredTime() {
        return storedTime;
    }
    public void setStoredTime(long storedTime) {
        this.storedTime = storedTime;
    }
    public int getUser() {
        return user;
    }
    public void setUser(int user) {
        this.user = user;
    }
}


//**********************************************************
//
// Http Server 
//
// Author: Bombo Gongo
//
// Class purpose: create and manage the session keys for the users
//
//*********************************************************

package org.bombo.data;

import java.util.Calendar;
import java.util.Iterator;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

import org.bombo.util.MathUtil;

public class SessionSingleton
{
    static private SessionSingleton singletonInstance;
    volatile private Map<String,Session> usedSessionKeys;
    private MathUtil calc;
    private long lastCleanup;

    private final long cleanupEverySecs;
    private final long maxSessionInSecs;

    private SessionSingleton() 
    {
        usedSessionKeys = new ConcurrentHashMap<String,Session>();
        Calendar cal = Calendar.getInstance();
        calc = new MathUtil();
        lastCleanup = cal.getTimeInMillis();
        cleanupEverySecs = 1000 * 60 * 15; // 15 minutes
        maxSessionInSecs = 1000 * 60 * 10; // 10 minutes
    }

    /*
     * Usage: return an instance of the singleton
     *    
     * Returns:
     *    Singleton instance
     */
    static public SessionSingleton getInstance()
    {
        if ( singletonInstance == null )
            singletonInstance = new SessionSingleton();

        return singletonInstance;
    }

    /*
     * Usage: return a new session key
     * 
     * Input:
     *    user = user id
     *    
     * Returns:
     *    null Error
     *    String containing a new session key in base 32
     */
    public String getSessionKey(int user) 
    {
        if (user < 0) return null;

        Calendar cal = Calendar.getInstance();
        Session session = new Session(cal.getTimeInMillis(), user);

        String sessionKey = calc.createSessionKey();
        usedSessionKeys.put(sessionKey, session);

        return sessionKey;
    }

    /*
     * Usage: validate a session key and check if a session is still valid
     * 
     * Input:
     *    sessionKey = base 32 session key string
     *    
     * Returns:
     *    -1 Error
     *    user id associated to the session
     */
    public int validateSessionKey(String sessionKey) 
    {
        if (sessionKey == null || sessionKey == "")
            return -1;

        Calendar cal = Calendar.getInstance();
        Session session = usedSessionKeys.get(sessionKey);

        long currentTime = cal.getTimeInMillis();

        if (session == null) {
            // SessionKey not valid
            return -1;
        } else if (currentTime - session.getStoredTime() > maxSessionInSecs) {
            // Session elapsed
            return -1;
        }

        if (currentTime - lastCleanup > cleanupEverySecs)
            doCleanup();

        // Valid session
        return session.getUser();
    }

    /*
     * Usage: clean the list from unused session key
     */
    private void doCleanup() {
        Calendar cal = Calendar.getInstance();
        lastCleanup = cal.getTimeInMillis();
        Iterator<String> iter = usedSessionKeys.keySet().iterator();
        String key = "";

        while(iter.hasNext()) { // For each seassion...
            key = iter.next();
            if (lastCleanup - usedSessionKeys.get(key).getStoredTime() > maxSessionInSecs) {
                usedSessionKeys.remove(key);
            }           
        }
    }
}

Oh, and here are some unit tests, if they help:

package org.bombo.data;

import junit.framework.TestCase;

public class ScoreSingletonTest extends TestCase {

    public void testGetHighestScores() {

        ScoreSingleton.getInstance().insertScore(1, 14, 18);
        ScoreSingleton.getInstance().insertScore(12, 3, 12455);
        ScoreSingleton.getInstance().insertScore(18, 3, 3444);
        assertEquals("12=12455\r\n18=3444\r\n", ScoreSingleton.getInstance().getHighestScores(3));

        ScoreSingleton.getInstance().insertScore(12, 4, 88);
        ScoreSingleton.getInstance().insertScore(18, 4, 99);
        ScoreSingleton.getInstance().insertScore(18, 6, 34);
        assertEquals("18=99\r\n12=88\r\n", ScoreSingleton.getInstance().getHighestScores(4));

        ScoreSingleton.getInstance().insertScore(18, 44, 33);
        ScoreSingleton.getInstance().insertScore(12, 5, 111);
        ScoreSingleton.getInstance().insertScore(18, 5, 120);
        assertEquals("18=120\r\n12=111\r\n", ScoreSingleton.getInstance().getHighestScores(5));

        assertEquals(null, ScoreSingleton.getInstance().getHighestScores(88));      
    }

    public void testInsertScore() {

        assertEquals(0, ScoreSingleton.getInstance().insertScore(11, 12, 18));
        assertEquals(-1, ScoreSingleton.getInstance().insertScore(-11, 10, 8));
        assertEquals(-1, ScoreSingleton.getInstance().insertScore(11, -10, 8));
        assertEquals(-1, ScoreSingleton.getInstance().insertScore(11, 10, -8));
    }
}


package org.bombo.data;

import junit.framework.TestCase;

public class SessionsSingletonTest extends TestCase {

    public void testGetSessionKey() {

        for (int i=0; i < 100; ++i)
            assertEquals(true, SessionSingleton.getInstance().getSessionKey(5).matches("[0-9a-z]+"));

    }

    public void testValidateSessionKey() {
        String sessionKey = "";

        for (int i=0; i < 100; ++i) {
            sessionKey = SessionSingleton.getInstance().getSessionKey(6);
            assertEquals(6, SessionSingleton.getInstance().validateSessionKey(sessionKey));
        }
    }
}


package org.bombo.util;

import org.bombo.util.MathUtil;

import junit.framework.TestCase;

public class MathUtilTest extends TestCase {

    public void testCreateSessionKey() {
        MathUtil mathUtil = new MathUtil();

        for (int i=0; i < 100; ++i)
            assertEquals(true, mathUtil.createSessionKey().matches("[0-9a-z]+"));

    }

    public void testGetPositiveInt() {
        MathUtil mathUtil = new MathUtil();

        assertEquals(15,mathUtil.getPositiveInt("15"));
        assertEquals(18,mathUtil.getPositiveInt("18"));
        assertEquals(1199,mathUtil.getPositiveInt("1199"));
        assertEquals(0,mathUtil.getPositiveInt("0"));

        assertEquals(-1,mathUtil.getPositiveInt("abc"));
        assertEquals(-1,mathUtil.getPositiveInt("-188"));
        assertEquals(-1,mathUtil.getPositiveInt("12.5"));
        assertEquals(-1,mathUtil.getPositiveInt("0,33"));

    }

}


package org.bombo.util;

import java.io.UnsupportedEncodingException;
import java.util.HashMap;
import java.util.Map;

import junit.framework.TestCase;

public class MiscUtilsTest extends TestCase {

    public void testParseQuery() throws UnsupportedEncodingException {

        MiscUtils utils = new MiscUtils();
        Map<String, Object> parameters = new HashMap<String, Object>();

        utils.parseQuery("name=Ale&surname=bobo", parameters);      
        assertEquals("Ale", parameters.get("name"));

        utils.parseQuery("request=login&surname=bobo", parameters);     
        assertEquals("login", parameters.get("request"));

        utils.parseQuery("name=Ron&levelid=15", parameters);        
        assertEquals("15", parameters.get("levelid"));
        assertEquals("Ron", parameters.get("name"));

        parameters.clear();

        utils.parseQuery("", parameters);       
        assertEquals(null, parameters.get("name"));

    }

}

Answer 1

1. Please think twice before using your own http server. The better way is using Jetty or embed Tomcat - good way to get rid of defects and to save time for developing business logic.

2. If you're going to use multithreading please look into java.util.concurrent.atomic package in order to start using AtomicInteger etc.

3. Your multithreading is not actually multithreading as you've wrapped all logic into synchronized block. This will cause all threads to be executed one by one.

4. Map<Integer,Map<Integer,List<Integer>>> is not a good way of handling variables. This might be an object. Probably you can make it thread safe inside

5. volatile on Map is useless. It's like putting final on it. Anyway usage of these modifiers is not recommended as you can handle all types by concurrent package.

6. This point is only my opinion but I do not like using TreeMap. As far as I see you are trying to return sorted String. But you can get values from scoreResults (map.values()) and sort them with Collections.sort. Also you should look into StringUtils.join() method (apache.commons package) in order to receive String. This will give you more readable code.

7. Always close streams in try catch with finally block and always consume response body as you can receive memory leaks in your code:

   os.write(response.toString().getBytes());

E.g. if the line above will cause an exception then os stream won't be closed. This will cause memory leak in your server. Please take a look in IOUtils.closeQuitely() method (apache.commons.io) as an example

Hope this will help you

Answer 2

Volatile

I don't have time to write much here, but I did want to comment on the use of volatile members in both of your singletons.

In my experience, the majority of the time that I see volatile in code, it's being misused. volatile does not make a member thread-safe, in the way that you want it to.

See more about Java volatile here

In your case, if multiple threads are going to be reading and writing data from the Map at the same time, ConcurrentHashMap will probably help you. But, volatile does not.

You are only assigning the users and usedSessionKeys map variables (e.g. users =) in the constructors of the classes that own them. So, there's no need to worry about whether or not one thread is assigning a new map to those members at (about) the same time that another thread is trying to access the member, where it might have to worry about having a stale value for the reference to the map.

Singleton

I haven't fully audited your code, but if you will have multiple threads calling getInstance() on your singleton classes, then as @Andrew said, you'll want to look into a thread-safe singleton pattern. There's tons online, so just search for thread-safe Java Singleton pattern. (for example, this one)

More

See this stack overflow answer for more related discussion