1
\$\begingroup\$

I have implemented a simple HTTP server which can process GET and POST requests from the client. I have written it in Java. For the client part, I just simply type in localhost:8080(file_name_here.file_type_here) in the browser. For the GET request, the server tries to find the requested file in the directory. If it finds it successfully, it shows the file on the browser and shows an error message if it fails to do so. For the POST request the server simply collects a username from a HTTP post form and shows the username in a HTML page. Any sort of constructive criticism on the code design or structure will be appreciated.

import java.io.*;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.Date;

public class HTTPServer {
    static Writer writer;
    private static ServerSocket serverConnect;
    private static final int PORT = 8080;
    static int id = 0;

    private static void serverSocketCreate(){
        try {
            serverConnect = new ServerSocket(PORT);
        } catch (IOException e) {
            e.printStackTrace();
        }
        System.out.println("Server started.\nListening for connections on port : " + PORT + " ...\n");
    }

    private static void logFileCreate(){
        File logFile = new File("log.txt");
        boolean result = false;
        try {
            result = Files.deleteIfExists(logFile.toPath());
            if(result) logFile.createNewFile();
            writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(logFile), StandardCharsets.UTF_8));
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public static void main(String[] args) throws IOException {
        serverSocketCreate();
        logFileCreate();
        while(true)
        {
            Socket s = serverConnect.accept();
            WorkerThread wt = new WorkerThread(s);
            Thread t = new Thread(wt);
            t.start();
            System.out.println("Thread number is: " + id);
        }
    }
}

class Information{
    private String method;
    private String fileName;
    private String fileType;

    public Information(){
        method = fileName = fileType = "";
    }

    public String getMethod() {
        return method;
    }

    public void setMethod(String method) {
        this.method = method;
    }

    public String getFileName() {
        return fileName;
    }

    public void setFileName(String fileName) {
        this.fileName = fileName;
    }

    public String getFileType() {
        return fileType;
    }

    public void setFileType(String fileType) {
        this.fileType = fileType;
    }
}

class WorkerThread implements Runnable{
    private Socket s;
    private BufferedReader in;
    private DataOutputStream out;

    private static String MIME_TYPE;
    private static final String SUCCESS_HEADER = "HTTP/1.1 200 OK\r\n";
    private static final String ERROR_HEADER = "HTTP/1.1 404 Not Found\r\n";
    private static final String OUTPUT_HEADERS = "Content-Type: " + MIME_TYPE  + "\r\nContent-Length: ";
    private static final String OUTPUT_END_OF_HEADERS = "\r\n\r\n";
    private static final String FILE_NOT_FOUND = "<html>\n<head>\n<title>\nError\n</title>\n</head>\n<body>\n<p>\n<h1>404-File Not Found</h1>\n</p>\n</body>\n</html>";

    public WorkerThread(Socket s){
        this.s = s;
        try{
            in = new BufferedReader(new InputStreamReader(s.getInputStream()));
            out = new DataOutputStream(s.getOutputStream());
        } catch (IOException e){
            e.printStackTrace();
        }

        HTTPServer.id++;
    }

    private void closeConnection() throws IOException{
        out.flush();
        out.close();
        in.close();
        s.close();
        //HTTPServer.writer.close();
    }

    private void sendData(byte [] data) throws IOException{
        out.writeBytes(SUCCESS_HEADER);
        out.writeBytes(OUTPUT_HEADERS);
        out.write(data.length);
        out.writeBytes(OUTPUT_END_OF_HEADERS);
        out.write(data);
    }

    private void sendPostData(String POST_DATA) throws IOException{
        out.writeBytes(SUCCESS_HEADER);
        out.writeBytes(OUTPUT_HEADERS);
        out.write(POST_DATA.length());
        out.writeBytes(OUTPUT_END_OF_HEADERS);
        out.writeBytes(POST_DATA);
    }

    private void sendErrorMessage() throws IOException{
        out.writeBytes(ERROR_HEADER);
        out.writeBytes(OUTPUT_HEADERS);
        out.write(FILE_NOT_FOUND.length());
        out.writeBytes(OUTPUT_END_OF_HEADERS);
        out.writeBytes(FILE_NOT_FOUND);
    }

    private void setMimeType(String fileType){
        if(fileType.equals("html")){
            MIME_TYPE = "text/html";
        } else if(fileType.equals("png")){
            MIME_TYPE = "image/png";
        } else if(fileType.equals("pdf")){
            MIME_TYPE = "application/pdf";
        } else if(fileType.equals("jpg")){
            MIME_TYPE = "image/jpg";
        } else if(fileType.equals("jpeg")){
            MIME_TYPE = "image/jpeg";
        } else if(fileType.equals("bmp")){
            MIME_TYPE = "image/bmp";
        } else if(fileType.equals("tiff")){
            MIME_TYPE = "image/tiff";
        } else if(fileType.equals("tif")){
            MIME_TYPE = "image/tiff";
        } else if(fileType.equals("default")){
            MIME_TYPE = "text/html";
        }
    }

    private byte [] readFileIntoByteArray(File file) throws IOException{
        FileInputStream fileInputStream = new FileInputStream(file);
        byte [] data = new byte[(int) file.length()];
        fileInputStream.read(data);
        fileInputStream.close();
        return data;
    }

    private void writeToLogFile(String message,String statusCode,int fileSize) throws IOException {
        HTTPServer.writer.write(InetAddress.getByName("localHost").getHostAddress() + "--" + "[" + new Date().toString() + "] \"" + message + "\" " + statusCode + " " + fileSize);
        HTTPServer.writer.flush();
    }

    private void addNewLineToLogFile() throws IOException{
        HTTPServer.writer.write("\r\n");
        HTTPServer.writer.flush();
    }

    private String readRequest() throws IOException{
        return in.readLine();
    }

    private Information extractInformation(String message){
        Information information = new Information();
        information.setMethod(message.substring(0,message.indexOf(' ')));
        information.setFileName(message.substring(message.indexOf("/") + 1,message.lastIndexOf(' ')));
        if(information.getFileName().isEmpty()) information.setFileName("index.html");
        information.setFileType(information.getFileName().substring(information.getFileName().indexOf(".") + 1));
        return information;
    }

    private int contentLength() throws IOException{
        String str;
        int postDataI = -1;
        while((str = readRequest()) != null) {
            if(str.isEmpty()) break;
            final String contentHeader = "Content-Length: ";
            if(str.contains(contentHeader)){
                postDataI = Integer.parseInt(str.substring(contentHeader.length()));
            }
        }
        return postDataI;
    }

    private String userName(int postDataI) throws IOException{
        String USER_DATA = null;
        char [] charArray = new char[postDataI];
        in.read(charArray);
        USER_DATA = new String(charArray);
        return USER_DATA;
    }

    private String modifyUserName(String USER_DATA){
        USER_DATA = USER_DATA.replaceAll("\\+"," ");
        USER_DATA = USER_DATA.substring(USER_DATA.indexOf("=") + 1);
        return USER_DATA;
    }

    @Override
    public void run() {
        try{
            String message = readRequest();

            Information info = new Information();

            if(message != null){
                info = extractInformation(message);
            }

            if(info.getMethod().equals("GET")){
                File file = new File(info.getFileName());

                // image format - bmp,jpg,png,tiff
                // file type - pdf , html

                if(file.isFile()){ // check if the file exists in the directory
                    // send the file
                    setMimeType(info.getFileType());
                    byte [] data = readFileIntoByteArray(file);
                    sendData(data);
                    writeToLogFile(message,"200",data.length);
                } else{
                    // file not found - 404
                    setMimeType("default");
                    sendErrorMessage();
                    writeToLogFile(message,"404",FILE_NOT_FOUND.length());
                }
            } else if(info.getMethod().equals("POST")){
                // read the post data
                int postDataI = contentLength();
                String USER_DATA = userName(postDataI);
                USER_DATA = modifyUserName(USER_DATA);
                if(!USER_DATA.isEmpty()){
                    String POST_DATA = "<html>\n<head>\n<title>\nPost Request\n</title>\n</head>\n<body>\n<p>\n<h1>" + USER_DATA +"</h1>\n</p>\n</body>\n</html>";
                    sendPostData(POST_DATA);
                    writeToLogFile(message,"200",USER_DATA.length());
                } else{
                    // blank username
                    String POST_DATA = "<html>\n<head>\n<title>\nPost Request\n</title>\n</head>\n<body>\n<p>\n<h1>Blank User Name</h1>\n</p>\n</body>\n</html>";
                    sendPostData(POST_DATA);
                    writeToLogFile(message,"200",0);

                }
            }
            addNewLineToLogFile();
            closeConnection();
        } catch (IOException e){
            e.printStackTrace();
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Without testing this myself, it looks rather buggy. Could you edit the question to describe the tests you've performed? \$\endgroup\$ Nov 9, 2018 at 9:27
  • \$\begingroup\$ It is for an assignment. The problem description can be found here: drive.google.com/file/d/1mi_wflWCQTAOToxo8Pu9ZVHY51YdDWxJ/… \$\endgroup\$
    – Robur_131
    Nov 9, 2018 at 10:09
  • \$\begingroup\$ There's nothing much to test here. I put the file names in the browser and it loads the file (as html,image format, pdf etc.). \$\endgroup\$
    – Robur_131
    Nov 9, 2018 at 10:10
  • \$\begingroup\$ Could you point out the possible pitfalls in my code? @PeterTaylor \$\endgroup\$
    – Robur_131
    Nov 9, 2018 at 10:11

2 Answers 2

0
\$\begingroup\$
public class HTTPServer {
    static Writer writer;
    private static ServerSocket serverConnect;
    private static final int PORT = 8080;
    static int id = 0;

PORT is fine. The other three should probably not be fields. The socket and connection count should have a scope limited to a single method, so don't need to be fields, and the logger could be passed into the classes which need it more elegantly, in a way which allows you to e.g. extend the code to support separate instances for separate sites with their own logs.

To implement the proposal above, serverSocketCreate should return the socket rather than storing it in a field.


        File logFile = new File("log.txt");

Why is this path not a static final field alongside PORT?


        boolean result = false;
        try {
            result = Files.deleteIfExists(logFile.toPath());
            if(result) logFile.createNewFile();
            writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(logFile), StandardCharsets.UTF_8));

I thoroughly approve of explicitly specifying UTF-8 as the encoding rather than relying on defaults. But the rest seems rather confused. It attempts to delete the file; if it succeeds, it creates a blank file; then it opens the file to write, truncating it. Why bother with the first two steps? The only reason I can guess is an attempt to eliminate some classes of security issue which can occur if the file already exists and has unwanted permissions; but in that case you would need to createNewFile() regardless of result.


    public static void main(String[] args) throws IOException {
        serverSocketCreate();
        logFileCreate();
        while(true)
        {
            Socket s = serverConnect.accept();

What if serverSocketCreate failed, catching and printing an IOException? What if logFileCreate() failed?


            WorkerThread wt = new WorkerThread(s);
            Thread t = new Thread(wt);
            t.start();
            System.out.println("Thread number is: " + id);

What exactly is the point of id? It doesn't seem to actually be used as an identifier.


class Information{

That is not a very informative name. Information about what?

    private String method;
    private String fileName;
    private String fileType;

    public Information(){
        method = fileName = fileType = "";
    }

In practical usage, the setters are used only immediately after creation. The values could instead be arguments of the constructor, and the fields could be public final, simplifying the class and giving the compiler and the maintenance programmer some useful hints.


class WorkerThread implements Runnable{

Again, not a very informative name.

    private static String MIME_TYPE;
    ...
    private static final String OUTPUT_HEADERS = "Content-Type: " + MIME_TYPE  + "\r\nContent-Length: ";

Spot the bug.

    private static final String SUCCESS_HEADER = "HTTP/1.1 200 OK\r\n";
    private static final String ERROR_HEADER = "HTTP/1.1 404 Not Found\r\n";

HTTP/1.1? I don't think so. This server doesn't even implement all of HTTP/1.0.

    private static final String FILE_NOT_FOUND = "<html>\n<head>\n<title>\nError\n</title>\n</head>\n<body>\n<p>\n<h1>404-File Not Found</h1>\n</p>\n</body>\n</html>";

Those \n are unnecessary and just make the content harder to read.


    public WorkerThread(Socket s){
        this.s = s;
        try{
            in = new BufferedReader(new InputStreamReader(s.getInputStream()));
            out = new DataOutputStream(s.getOutputStream());
        } catch (IOException e){
            e.printStackTrace();
        }

Is this the right place to catch that exception, and if so is that the right way to handle it? The result is that you end up with a useless object, and you're going to spin up a thread just to throw a NullPointerException.


    private void closeConnection() throws IOException{
        out.flush();
        out.close();
        in.close();
        s.close();
        //HTTPServer.writer.close();
    }

Here, on the other hand, catching some exceptions would be useful. If you fail to close out, you may still be able to tidy up some native state by closing in.


    private void setMimeType(String fileType){
        if(fileType.equals("html")){
            MIME_TYPE = "text/html";
        } else if(fileType.equals("png")){
            MIME_TYPE = "image/png";
        } else if(fileType.equals("pdf")){

Here a Map<String, String> set up in a static constructor would be a lot cleaner.


    private byte [] readFileIntoByteArray(File file) throws IOException{
        FileInputStream fileInputStream = new FileInputStream(file);
        byte [] data = new byte[(int) file.length()];
        fileInputStream.read(data);

This is badly buggy. Read the API doc for FileInputStream.read and then fix it by using the return value and a loop.


    private void writeToLogFile(String message,String statusCode,int fileSize) throws IOException {
        HTTPServer.writer.write(InetAddress.getByName("localHost").getHostAddress() + "--" + "[" + new Date().toString() + "] \"" + message + "\" " + statusCode + " " + fileSize);

Firstly, this has a race condition. Multiple threads accessing a single writer need to be synchronised.

Secondly, what does InetAddress.getByName("localHost").getHostAddress() give? That could use a comment, and possibly could be cached.


    private String readRequest() throws IOException{
        return in.readLine();

See my previous comments about not being HTTP/1.1. It's impossible to implement HTTP/1.1 reading only a single line of the request.


    private Information extractInformation(String message){

Is this really parseRequest?


    private int contentLength() throws IOException{
        String str;
        int postDataI = -1;
        while((str = readRequest()) != null) {
            if(str.isEmpty()) break;
            final String contentHeader = "Content-Length: ";
            if(str.contains(contentHeader)){
                postDataI = Integer.parseInt(str.substring(contentHeader.length()));
            }
        }

Yikes! Why not read all of the headers into some structure, preserve the whole structure, and extract individual headers when needed?


            String message = readRequest();

            Information info = new Information();

            if(message != null){
                info = extractInformation(message);
            }

If message == null shouldn't there be an error? And in that case, why create an empty Information instance which will immediately be discarded?


            if(info.getMethod().equals("GET")){
                File file = new File(info.getFileName());

This is the classic security flaw of HTTP servers: by failing to properly validate the filename, it opens up the entire filesystem of the computer. Consider e.g. a request GET //etc/passwd HTTP/1.0. The parsing of extractInformation will give filename /etc/passwd, and this will return that file, which is very useful for hackers.

This is the biggest flaw in the code, by far.


                    byte [] data = readFileIntoByteArray(file);
                    sendData(data);

That's not a good idea. What if the file is 50GB? Do you have 50GB of RAM? Loop: read a chunk into a buffer, write the chunk into the output stream. Or there may be some more sophisticated way of doing the same thing with NIO.


                    // file not found - 404
                    setMimeType("default");
                    sendErrorMessage();
                    writeToLogFile(message,"404",FILE_NOT_FOUND.length());

default isn't a filetype. If you're returning an HTML body, the MIME type of the header should be text/html.


            } else if(info.getMethod().equals("POST")){

I'm not even going to go there. Whatever this is, it's so specific to some particular use case that I don't understand what it's doing in this HTTP server.


What about methods other than GET and POST? Do you just drop the connection without writing any kind of error response?

\$\endgroup\$
0
\$\begingroup\$
  1. You are not limiting the number of worker threads which are getting created. Hence, if there is a flood of requests, you website may go down.
  2. Consider using ExecutorService which creates a pool of reusable threads, rather creating a new Thread for every request.
  3. Lastly connections should always be closed within a finally block. Currently, you are doing it inside the try block, thus, in case of any exception, you will leave open connections which can bring your server down
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.