Webserver for handling GET and HEAD requests

Question

I have a programming assignment where I need to build a webserver which can handle GET and HEAD requests. My webserver is working as it should but since I'm still a newbie at Java I'm not sure about my structure or quality of my code so if you have some criticism/tips I would appreciate it.

Main:

package pytteweb;

import java.net.ServerSocket;
import java.net.Socket;


public class PyttewebMain {

    public static void main(String[] args) {

        int port = 8080;

        if(args.length == 1){

            try{
                port = Integer.parseInt(args[0]);
            }
            catch (Exception e) {
                System.err.print("Something went wrong..check your arguments \nUsage: pytteweb [port]\n");
                System.exit(1);
            }
        }

        else if(args.length > 1){
            System.err.print("Usage: pytteweb [listening port]");
            System.exit(1);
        }

        try{

            ServerSocket listen = new ServerSocket(port);   

            System.out.print("Welcome to the pytteweb 1.0 web server..listening for connections on port " + port + "...\n");

            while(true) {   

                Socket clientSocket = listen.accept();

                System.out.println("\nNew connection from " + clientSocket.getInetAddress());

                RequestHandler requestHandler = new RequestHandler(clientSocket);

                Thread thread = new Thread(requestHandler);

                thread.start(); 
            }
        }

        catch (Exception e) {
            System.err.print("Something went wrong.. port " + port + " already in use? \n");
            e.printStackTrace();
            System.exit(1);

        }

    }

}

RequestHandler:

package pytteweb;

import java.net.Socket;
import java.util.StringTokenizer;
import java.io.*;

public class RequestHandler implements Runnable {

    Socket clientSocket = null;
    String[] requestLines = null;
    String requestLine = "";
    String httpMethod = "";
    String httpVersion = "";
    String fileName = "";
    String status = "";
    String END = "\r\n";
    boolean badRequest = true;

    public RequestHandler(Socket socket) throws Exception   {
        this.clientSocket = socket;
    }

    public void run(){
        try {
            processRequest();
        }
        catch (Exception e){
            System.out.println("Error trying to process request..");
            System.out.println(e);
            e.printStackTrace();
        }
    }

    /**
     * Processes the request sent from the client. Reads and checks the request. Uses the Responder class to respond. 
     * 
     * 
     */
    private void processRequest() throws Exception {

        System.out.println("\nReading request..\n");
        requestLines = readRequest(clientSocket).split(System.getProperty("line.separator"));
        requestLine = requestLines[0].toUpperCase();

        for (String s: requestLines) {           
            System.out.println(s); 
        }

        if(requestLine.startsWith("QUIT")){
            System.exit(0);
        }

        StringTokenizer st = new StringTokenizer(requestLine);

        if(requestLine.startsWith("GET") || requestLine.startsWith ("HEAD") && requestLine.endsWith("HTTP/1.0") || requestLine.endsWith("HTTP/1.1")){

            badRequest = false;

            if(st.countTokens() < 3){
                badRequest = true;
            }       
        }

        if(!badRequest){

            httpMethod = st.nextToken();
            fileName = st.nextToken();
            httpVersion=st.nextToken();

        }

        Responder r = new Responder(httpMethod,fileName,clientSocket,badRequest);
        r.sendResponse();
        clientSocket.close();

    }

    /**
     * Reads the request. 
     * 
     * @param s - the socket to read from.
     * @throws IOException
     */
    private String readRequest(Socket s) throws IOException{

        InputStream input = clientSocket.getInputStream();

        int avaliable = 0;
        String received = "";

        while (!received.endsWith(END + END)) {
            avaliable = input.available();

            byte[] bytes = new byte[avaliable];
            input.read(bytes);

            received += new String(bytes);
        }
        return received.trim();
    }

}

Responder:

package pytteweb;

import java.io.DataOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.OutputStream;
import java.net.Socket;

public class Responder {

    String status = null;
    String contentType = null;
    String fileName = null;
    String httpMethod = "";
    String END = "\r\n";
    String contentLength = null;
    DataOutputStream out = null;
    File file = null;
    FileInputStream fis = null;
    boolean badRequest = false;


    public Responder(String httpMethod, String fileName, Socket clientSocket, boolean badRequest){

        this.httpMethod = httpMethod;
        this.fileName = fileName;
        this.badRequest = badRequest;

        try {
            this.out = new DataOutputStream(clientSocket.getOutputStream());
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

    }

    /**
     * A method to send the correct response 
     * @throws Exception 
     * 
     * 
     */
    public void sendResponse() throws Exception{

        if(badRequest){
            System.out.println("\n400 Bad Request\n");
            status = "HTTP/1.1 400 Bad Request" + END;
            fileName = "ERROR400.HTML";
        }

        else if(!fileExists()){
            System.out.println("\n404 Not Found: \n" + fileName + "Was not found on this server...");
            status = "HTTP/1.1 404 Not Found" + END;
            fileName = "ERROR404.HTML";
        }
        else if(fileExists() && !badRequest){
            status = "HTTP/1.1 200 OK" + END;
        }

        file = new File(fileName);

        try {
            fis = new FileInputStream(file);
        } catch (FileNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

        contentType = "Content-type: " + getContentType(fileName) + END;
        contentLength = "Content-Length: " + file.length() + END;

        out.writeBytes(status);
        out.writeBytes(contentType);
        out.writeBytes(contentLength);
        out.writeBytes(END);    

        if(httpMethod.equalsIgnoreCase("GET")){
            sendBytes(fis, out);
        }

        fis.close();
        out.close();

    }

    /**
     * A method to check if a file exists and also reformats if needed
     * 
     * 
     * @return True if the file exists and false if it does not.
     */
    private boolean fileExists(){

        if(fileName.endsWith("/")){
            fileName+="INDEX.HTML";
        }

        if(fileName.startsWith("/")){
            fileName = fileName.toUpperCase().substring(1);
        }

        //if(fileName.contains("favicon")){
        //  return false;
        //}

        File file = new File(fileName);
        if(!file.exists()){
            return false;
        }

        return true;    

    }

    /**
     * A method to check and return what kind of file is requested by the client.
     * 
     * @param fileName specifies the filename to check.
     * @return The file type of the file.
     */
    private String getContentType(String fileName)
    {
        if(fileName.endsWith(".TXT")) {return "text/plain";}
        if(fileName.endsWith(".HTM") || fileName.endsWith(".HTML")) {return "text/html";}
        if(fileName.endsWith(".JPG") || fileName.endsWith(".JPEG")) {return "image/jpeg";}
        if(fileName.endsWith(".PNG")) {return "image/png";}
        if(fileName.endsWith(".GIF")) {return "image/gif";}
        if(fileName.endsWith(".ICO")) {return "image/ico";}
        return "application/octet-stream";
    }

    /**
     * A method which buffers data from a fileinputstream and sends it to the outputstream.
     * 
     * @param in - The FileInputStream to be read
     * @param out - The OutPutStream to be written to
     */
    private void sendBytes(FileInputStream in, OutputStream out) throws Exception{
        byte[] buffer = new byte[1024];
        int bytes = 0;

        while((bytes = in.read(buffer)) != -1 )
        {
            out.write(buffer, 0, bytes);
        }   
    }
}

Answer 1

Take a careful look at PyttewebMain.main(), and try to identify the different things that it is doing. In particular, it seems to be serving as a usage validator, two different kinds of factories, a run loop..., maybe more.

In particular

            Socket clientSocket = listen.accept();
            System.out.println("\nNew connection from " + clientSocket.getInetAddress());
            RequestHandler requestHandler = new RequestHandler(clientSocket);
            Thread thread = new Thread(requestHandler);
            thread.start(); 

Notice that this section of code doesn't care at all about the command line arguments. This bit of code only cares about the socket that has been passed to it. That strongly suggests that there's a distinct object lurking in here.

class WebServer {
    private final ServerSocket listen;

    void acceptConnection () {
            Socket clientSocket = listen.accept();
            System.out.println("\nNew connection from " + clientSocket.getInetAddress());
            RequestHandler requestHandler = new RequestHandler(clientSocket);
            Thread thread = new Thread(requestHandler);
            thread.start(); 
    }
}

If you look carefully here, you'll see that there are still two different ideas - accepting the request and handling the request. At a minumum, we should be teasing those out to different methods.

class WebServer {
    private final ServerSocket listen;

    void acceptConnection () {
            Socket clientSocket = listen.accept();
            respond(clientSocket);
    }

    void respond(Socket clientSocket) {
            System.out.println("\nNew connection from " + clientSocket.getInetAddress());
            RequestHandler requestHandler = new RequestHandler(clientSocket);
            Thread thread = new Thread(requestHandler);
            thread.start(); 
    }
}

Now, it can be the right thing to write your own Thread management. But if you're just getting started, it probably isn't. Your default tool here should be to use an ExecutorService to launch your Runnables.

class WebServer {
    private final ServerSocket listen;
    private final ExecutorService executor;

    void acceptConnection () {
            Socket clientSocket = listen.accept();
            respond(clientSocket);
    }

    void respond(Socket clientSocket) {
            System.out.println("\nNew connection from " + clientSocket.getInetAddress());
            RequestHandler requestHandler = new RequestHandler(clientSocket);
            executor.submit(requestHandler);
    }
}

We still need the loop for continuing to accept requests as they arrive at the socket. Since we are, in some sense, iterating over the ServerSocket, it's not going to surprise anyone if the loop is included here.

class WebServer {
    ...
    public void run () {
        while(true) {
            acceptConnection();
        }
    }
    ...
}

OK, let's look at how you handle the requests.

public class RequestHandler implements Runnable {

    Socket clientSocket = null;

    public RequestHandler(Socket socket) throws Exception   {
        this.clientSocket = socket;
    }

You aren't going to be changing the clientSocket of an individual request, are you? The variable should be final. It maybe also should be private (it's not really part of the api, after all).

public void run(){
    try {
        processRequest();
    }
    catch (Exception e){
        System.out.println("Error trying to process request..");
        System.out.println(e);
        e.printStackTrace();
    }
}

I think it would have been acceptable here to implement your request processing here in the run method. processRequest isn't a particularly great name for what you are doing, but it is better than run, so hard to say.

Also, this is a pretty naive error handling strategy. Writing text data to a PrintStream isn't particularly flexible. You should also notice that, in writing this, you are baking your error processing logic into your RequestProcessor. Passing the error off to a dedicated listener would be cleaner.

private String readRequest(Socket s) throws IOException{

    InputStream input = clientSocket.getInputStream();

    int avaliable = 0;
    String received = "";

    while (!received.endsWith(END + END)) {
        avaliable = input.available();

        byte[] bytes = new byte[avaliable];
        input.read(bytes);

        received += new String(bytes);
    }
    return received.trim();
}

Anytime you find yourself concatenating a String in a loop, you should consider whether StringBuilder.append() is more suitable. String in java is immutable, so concatenation in a loop really means "make lots of copies of the beginning of the string".

Also, as a rule you should declare variables in the scope where they are used.

    while (!received.endsWith(END + END)) {
        int avaliable = input.available();

And check your spelling.

private void processRequest() throws Exception {

    System.out.println("\nReading request..\n");
    requestLines = readRequest(clientSocket).split(System.getProperty("line.separator"));
    requestLine = requestLines[0].toUpperCase();

    for (String s: requestLines) {           
        System.out.println(s); 
    }

    if(requestLine.startsWith("QUIT")){
        System.exit(0);
    }

I want to call your attention to two different things here.

First

    if(requestLine.startsWith("QUIT")){
        System.exit(0);
    }

Calling System.exit in a processor thread is probably not a great idea, even when you aren't deliberately building in a back door denial of service attack into your application. Interruption and Cancellation are complicated topics; if you are expecting to be writing robust multithreaded applications, then you should study the topic in detail. In a throw away project, you should just send a SIG_QUIT to the app when you want it to shut down.

Second, you should notice that you've got a lot of complicated logic for handling the HTTP request buried in here, but you can't get at it very easily via the interface you have. Specifically, you've got a Parser for the request header buried in here with the socket handler. If you were going to write tests to make sure that different requests were handled correctly, you might not want to have to force all the data into a client socket first. Pulling the parser logic out into a separate method/class would make writing tests much easier.

Also, note that you have a bunch of state variables disguised as member variables, even though they are only used in one method. There's nothing wrong with an object carrying around its own state (although again, since that state isn't part of the interface, the variables should be private); but state that is only referenced from a single scope should be declared in that scope.