Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to implement a HTTP Server in Java, I want the server to use a persistent connection per thread for request and response. After some research on Google, this is how my program looks like. It runs fine without any exceptions but I am unable to tell if it is doing what I want it to do, that is connection per thread policy. Can some one please take a look at it and tell me if there any changes to be made or if there are any flaws in my program?

public class TinyHttpd4ServerThread extends Thread{
    Socket client;

    String line = null;
    String httpVersion = null;
    boolean connectionKeepAlive;
    private ArrayList files = new ArrayList();
    ArrayList lineArray = new ArrayList();
    ArrayList tokens = new ArrayList();
    ArrayList requestHeaderLines;
    StringTokenizer st = null;
    ReentrantLock lock = new ReentrantLock();
    TinyHttpd4ServerThread(Socket socket)
    {
        this.client = socket;
    }

    public void run()
    {
        try
        {
            lock.lock();
            executeCommand(client);
        }
        catch(Exception e)
        {
            e.printStackTrace();
        }
        finally
        {
            lock.unlock();
        }

    }

    private void executeCommand( Socket client ){
        try {
            try {

                String fileName = "index.html";
                String token = null;                
                client.setSoTimeout(30000);
                BufferedReader in = new BufferedReader( new InputStreamReader( client.getInputStream() ) );  
                PrintStream out = new PrintStream( client.getOutputStream() );              
                System.out.println( "I/O setup done" );             
                String line = in.readLine();    //Get first line of request to check the request command GET or HEAD            
                StringTokenizer tokenLine = new StringTokenizer(line);
                String reqCommand = tokenLine.nextToken();              
                if (reqCommand.equals("GET") || reqCommand.equals("HEAD")) //only GET or HEAD requests are accepted, else gives 501 error
                {
                  String fname = tokenLine.nextToken();
                  if(fname.startsWith("/"))
                  {
                      while( line != null ) 
                      {
                         // requestHeaderLines.add(line);
                          System.out.println(line);
                          if(line.equals("")) break;
                          StringTokenizer tokenizedLine = new StringTokenizer(line);
                          while(tokenizedLine.hasMoreTokens())
                          {
                              token = tokenizedLine.nextToken();
                              if(token.endsWith(".html")||token.endsWith(".jpg")) //checking for file types.
                                  files.add(token);
                              if(token.startsWith("HTTP")) //checking version of the HTTP connection.
                                  if(token.contains("/1.0"))
                                      httpVersion = "HTTP/1.0";
                                  else if(token.contains("/1.1"))
                                      httpVersion = "HTTP/1.1";                               
                              if(token.equals("Connection:")) //parsing request to get connection type
                                  if(tokenizedLine.nextToken().equals("keep-alive")) //checking for connection type
                                      connectionKeepAlive = true;
                          } 
                          line = in.readLine();
                      }
                      System.out.println(line);         
                      File file = new File(fileName).getAbsoluteFile();
                      if(!(file.exists()))
                      {
                          out.println("Error Code: 404 \n File not found error...");
                      }
                      else
                      {
                          System.out.println(file.getName() + " requested.");
                          System.out.println("Files in this request are: ");
                          Iterator it = files.iterator();
                          while(it.hasNext())
                          {
                              System.out.println(it.next().toString());
                          }
                          sendFile(out, file);
                      }
                 }
                 else
                 {
                      out.println("Error Code: 400 \n Bad Request....");
                 }
               }
               else
               {
                   out.println("Error code: 501 \n Not Implemented Error....");  
               }
               out.flush();
               //out.close();
               //in.close();

               if(client.isClosed())
                   System.out.println("client is closed before finally");

            }finally {
                if(httpVersion.equals("HTTP/1.0") && connectionKeepAlive == false)
                {
                    client.close();
                    System.out.println( "HTTP/1.0 and Keep-alive = false: Connection is closed." );
                }
                else if(httpVersion.equals("HTTP/1.0") && connectionKeepAlive == true)
                {
                    client.setKeepAlive(true);
                    System.out.println("client keep alive: "+client.getKeepAlive());
                    client.setSoTimeout(5000);
                    System.out.println("So_timeout set: "+client.getSoTimeout());
                }
                else if(httpVersion.equals("HTTP/1.1"))
                {
                    client.setKeepAlive(true);
                    System.out.println("client keep alive: "+client.getKeepAlive());
                    client.setSoTimeout(5000);
                    System.out.println("So_timeout set: "+client.getSoTimeout());
                }
            }
        }
        catch(SocketTimeoutException e)
        {
            try {
                client.close();
            } catch (IOException e1) {
                // TODO Auto-generated catch block
                e1.printStackTrace();
            }
            System.out.println("Socket timeout, Connection closed.");
        }
        catch(Exception exception) {
            exception.printStackTrace();
        }
    } 


    private void sendFile(PrintStream out, File file){
        try{
            Date date = new Date();
            DateFormat dateFormat = new SimpleDateFormat("yyyy/MM/dd HH:mm:ss");
            out.println(httpVersion+" OK");
            out.println("Content-Type: text/html");
            out.println("Date: "+dateFormat.format(date));
            int len = (int) file.length();
            out.println("Content-Length: " + len);
            out.println("");  

            DataInputStream fin = new DataInputStream(new FileInputStream(file));
            byte buf[] = new byte[len];

            fin.readFully(buf);
            out.write(buf, 0, len);
            out.println("/nFiles in this request are: ");
            Iterator it = files.iterator();
            while(it.hasNext())
            {
                out.println(it.next().toString());
            }
            out.flush();
            fin.close();
        }catch(IOException exception){
            exception.printStackTrace();
        }         
    }
}

The finally clause uses the client.setSoTimeout() and this should throw a timeout exception after 5 sec. I want the connection to be closed when the timeout exception is thrown. but the connection is not closing. the time out exception is not being raised. I cannot understand what is wrong. can some one please tell me what is wrong and how to modify the finally clause such that if the HTTP version is 1.1 or the connection is keep alive then the setSoTimeout should be triggered for say 5 sec and if there is no request or response taking place in these 5 sec then the time out exception should be thrown and the connection should close.

share|improve this question
    
Do you have some restrictions about the Java version or something else ? –  Marc-Andre May 14 at 18:19
    
@ Marc. No I donot have any restrictions regarding java version. –  developer May 14 at 19:25
add comment

2 Answers 2

up vote 3 down vote accepted

Since you don't have any restrictions for the Java version, you can use some feature to enhanced the readability of the code.

Try-with-resources

You should really use this feature that is available since Java 7. The Oracle's tutorial is a really good one that will go in more details that I will. A quick summary of the tutorial is that the try-with-resource works mostly as before, but if your resource implements 'AutoCloseable' it will automatically close the resource for you. No more, final with close() and if ( resource != null) resource.close(), everything will be taken care of. The syntax is quite simple. Here is an example taken from your code.

try (BufferedReader in = new BufferedReader(new InputStreamReader(client.getInputStream()));
            PrintStream out = new PrintStream(client.getOutputStream());) {
    System.out.println("I/O setup done");
    String line = in.readLine(); // Get first line of request to check
    // the request command GET or HEAD
    // ...
} catch (Exception exception) {
    exception.printStackTrace();
}

Note that this will close the stream, so you must be sure that this is qhat you want, but if you implements something close to what @rolfl is suggesting, you will probably need it (since he's already using it).

StringTokeneizer

Reading documentation of a class can be very useful. Directly from the StringTokeneizer documentation :

StringTokenizer is a legacy class that is retained for compatibility reasons although its use is discouraged in new code. It is recommended that anyone seeking this functionality use the split method of String or the java.util.regex package instead.

The following example illustrates how the String.split method can be used to break up a string into its basic tokens:

String[] result = "this is a test".split("\\s");
 for (int x=0; x<result.length; x++)
     System.out.println(result[x]);

prints the following output:

this
is
a
test

I strongly suggest you that you drop the use of this class and convert it to use split().

One alternative to read the header is to parse everything and store it in a Map or something similar. In most library that I've worked with for HTTP request, the headers will be parse and then store in a Map. This make it easy to get value of specific parameter. It could looks like :

Map<String, String> httpHeader = request.getHttpHeader();

if(httpHeader.get("Connection").equals("keep-alive")) {
    connectionKeepAlive = true;
}
  • This example is not quite a good example since you should use some constants for Connection and keep-alive.

My personal suggestion would be to use a library that deal with all this stuff. There are good libraries that with deal elegantly with everything that could go wrong. Unless the goal of this program is to learn how to work directly with HTTP requests, using a library would be my way to go.

share|improve this answer
add comment

I do not believe this code is doing what you expect... and, some of the more important parts of the code are not included here.

There are a number of standard models that are used in Java to run Network-based servers. Your code does not follow any pattern I am familiar with.

First, some low-level concepts:

Thread

Do not extend Thread class. It is commonly done, but, it is not the right object-model to use. What you have is something that is Runnable, your class is not a Thread, but a Thread runs it. So, you want something to run..., then you want to run that runnable on the Thread. Your entire class should be renamed from public class TinyHttpd4ServerThread extends Thread{... to be:

public class TinyHttpdSocketHandler implements Runnable {

And, then when you create it, you should have the same Socket-based constructor:

TinyHttpdSocketHandler(Socket socket)
{
    this.client = socket;
}    

Now, with this runnable, you have to add it to a Thread. You do not include the code that does this for you, but, it should look something like:

Runnable handler = new TinyHttpdSocketHandler(socket);
Thread socketThread = new Thread(handler, "Thread for " + socket.toString());
socketThread.setDaemon(true);
socketThread.start();

Note how I have called setDaemon(true). This allows your application to exit in a sane way.

Encoding

The encoding in the data is determined by the protocol values. The actual headers and stuff in the HTTP protocol are 8-bit ASCII (I believe, check me on that). Your are loading up a default InputStreamReader on your InputStream and this may be messing with the encoding. You should probably keep things as ASCII, and as a Stream. Manually finding the newline and parsing from there...

BufferedInputStream bis = new BufferedInputStream(socket.getInputStream());
BufferedOutputStream bos = new BufferedOutputStream(socket.getOutputStream());

etc.

Game Loop

When designing games, they have what's called a "Game Loop". This is a system where the game iterates through and manages things, even when there is no input, etc. The system works well for network socket handling too.

You want to take the socket, expose the parts you need, then loop on it, and wait for activity. Your code is not doing that. It has no loop.

The request/response nature of the HTTP protocol should be very obvious in the loop. Something like:

public void run() {
    try (
        BufferedInputStream bis = new BufferedInputStream(socket.getInputStream());
        BufferedOutputStream bos = new BufferedOutputStream(socket.getOutputStream());) {

        boolean alive = true;
        while (alive) {
            alive = false;
            try {
                Response response = null;
                try {
                    Request req = parseRequest(bis);
                    response = processRequest(req);
                } catch (BadParseException bpe) {
                    response = newExceptionStatus(HTTP_400_BAD_REQUEST, bpe);
                } catch (BadProcessException bqe) {
                    response = newExceptionStatus(HTTP_500_INTERNAL_ERROR, bqe);
                }
                if (response == null) {
                    response = newExceptionStatus(HTTP_500_INTERNAL_ERROR, bqe);
                }
                returnRespone(response, bos);
                alive = true;                    
            } catch (Exception e) {
                // log this locally, we can't return it to the client....
                alive = false;
            }
        }
    }
}

Conclusion

With your system not having a decent game loop, with the socket being passed around in a funny way, and with the Thread being 'unusual', there are a lot of infrastructure things that should be changed. I recommend you read up on Java Network servers

share|improve this answer
    
@rolfi. thanks that answer is really helpful and you have explained it well to. –  developer May 14 at 20:39
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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