CS 5523 Operating Systems
Laboratory 1 Post Mortem
Proxy Monitor Spring 2003

Comments on InitialLine

General strategies for implementing InitialLine:
1. Strategy 1: store most of the parsed information in a URL object.
2. Strategy 2: do all parsing and validity testing in the constructor so that all of the get methods simply return values.
For the InitialLine class, I think Strategy 2 is better. Once the object is constructed, it is immutable. You don't have to worry about thread synchronization, and the logic is simpler. Common practice would be to have a private init function that is called from the constructor and contains most of the initialization code.
Some people ignored the parsing capabilities of the URL class and tried to do the parsing directly. Always use existing classes if they can do the job.
Bugs for InitialLine

Used Strategy 1, but didn't test for valid URL such as in:

   public String getHost() {
      return myURL.getHost();
   }

Didn't initialize the fields such as host prior to parsing. If the line is invalid, you should still have a strategy for returning something, since the InitialLine constructor doesn't throw an exception.
InitialLine constructor or other methods throw an exception (not the spec).
Allocated an array to hold the three tokens, and tokenized all of the tokens in the line without first checking that the line had at most 3 tokens. (This throws an array out of bounds exception.) A test input line of more than 3 tokens would have picked up this error.
Incorrect layout of try-catch for the URL:
```
     try {
         myurl = new URL(....
     } catch (MalformedURLException e) { }
     host = myurl.gethost();
     ...
```
If the URL is invalid, you do not want to use myurl to get the values. Those gets should be in the try-catch.

Testing
- Many people did not give adequate test cases for InitialLine.
- A good way to test is to output a message, what this test case was designed to test (i.e., too many tokens on the line) and what the expected result of the test should be. This makes your test program self-documenting.
- I would have liked to have seen a table of test cases in your report.

Comments on HTTPHeader
- Most people resigned themselves to reading one character at a time and didn't have the problems that last year's implementations had. However some people read one or more bytes past the empty line and its delimiters. Significantly, their later parts of the project didn't always correctly work.
- If you have output that has over-written characters in the header, chances are you have a bug in the way that you are handling line delimiters.
- Many people did not check to make sure that n was in range in the getHeaderLine method.
- Some people used the deprecated readLine method of DataInputStream in spite of instructions in class not to do so.
- Many people did not handle the case of limited line length correctly. The problems included the following.
- Some people used String where they should have used StringBuffer. For example the following causes a new String object to be allocated when each byte is read, and the entire string so far is copied.
```
     String line = new String("");
     while ( condition for carriage return) 
        line = line + in.readByte();
```
- Some people created a Byte object for each byte read in.
```
     while (....)
        Byte b = new Byte(in.readByte());
```
  This is unnecessary and inefficient.
- A couple of people created a Byte object for each byte read and then inserted each Byte object into their list or Vector instead of each line. A couple of other people did the same thing, creating a new StringBuffer for each character and putting that in the Vector for each byte read.
Comments on CopyFromTo
- Incorrect synchronization of the number of bytes read so far:
- Incorrect handling of the end of communication. Remember, unless you close the streams the underlying system resources are not released and the remote peers don't know that it is ended
Comments on Tunnel
- Incorrect handling of logic:
- Incorrect measurement of time --- There are two workable strategies for ending so that you can get the total time and have the server handle requests in parallel.
  - Strategy 1: Create a separate Connection thread that creates two CopyFromTo threads and then joins with them. Measure the ending time of each thread.
  - Strategy 2: Have one thread whose responsibility is cleanup. Each CopyFromTo thread puts itself on the cleanup thread's list. The cleanup thread takes threads off a list and calls one of their methods to get their time (like a call-back). This strategy is considerably more complicated, but might be used in practice for efficiency.
Comments on ProxyMonitor Most of the problems that were encountered here were due to bugs in the first four parts.
Comments on LoggingProxy
- Incorrect implementations:
- Some people created a separate Logger class to handle the output of the header. This is the preferred method.
- Many people had very complex if-else code segments with lots of repeated code to handle the three logging cases. An example of a more object-oriented design might define a base class Connection for no logging and subclasses for the different logging levels. Actually the "headers" case might be a superclass for the "all" case. With this approach, you just instantiate a different type of Connection object for the different types of logging. Alternatively, you could have a Connection class with multiple constructors, but you need to make sure that you avoid the complex logic.
Threading and timing errors:

Assumed that client sends the request and then server responds. This is not the specification. You must have two threads capable of simultaneously transferring information.
Assumed that the entire request could be read in a single read, provided the buffer was big enough. This only works on a fast local network. TCP provides an abstraction of a stream of bytes without packet or message boundaries. You have no control over how much will be delivered in a single read.
Did not measure the time to process the client requests in the tunnel program. The specification for the tunnel asked you to measure the time of the client communication.
Incorrectly measured the time to process the client requests. For example, measuring the start time after the accept and the end time after the thread creation. This placement measures thread creation time, and indicates to me that you do not understand thread flow of control. The end time should be measured after the join.
Measured time values that were completely unreasonable, such as a time in milliseconds that was equivalent to several years. Look at the output to see if it is reasonable.
Used sleep in the threads to inappropriately force synchronization. When I see sleep(1000) in random places in a program, I know it doesn't work right.
Logging one line at a time to the log file. The log file is a shared resource. To really do this right, you should accumulate the header in a StringBuffer. When the header is complete, the thread should write to the log file. Actually a good approach is to have a separate LogFile class and make its write method synchronized. (This still doesn't solve the problem of writing one header line at at time to the file.)
Connected to the destination web server from the tunnel before accepting a client connection. If you type fast, you might not notice a problem. In a real implementation, the destination web server will disconnect after a fairly short time period when no incoming request appears.

Bad parsing and logging strategies:
- Used indexOf("\n\r") to parse the header from the resource. In most cases, this approach resulted in taking the first line of the response instead of the entire header.
- Parsing inline in the main loop. When I see this, I know that you didn't test your parsing method very well--- how could you? You should remove your parsing code from the main loop, put it in a method, or better a class and test it separately. (The new specification mostly eliminated this problem, as most but not all, people called HTTPHeader to do the parsing.
- Treated the resource as a String. The difference between a byte and a character in Java is quite confusing.
Uncaught exceptions, badly caught exceptions, and bad exits:

Did not seriously or correctly address the problem of when the servers should exit. In general a client should NOT be able to cause a server to exit. The server should only exit if there is an irrecoverable error due to resources (memory, descriptors, etc.) that would jeopardize future correct execution. Remember the Mars Pathfinder! For this program, the only place the server should exit is when it fails to create a ServerSocket.
Exited on a bad accept or in random methods throughout the program.
Exited if the proxy fails to connect with the destination web server.
Allowed exceptions to go uncaught. An uncaught exception anywhere causes the program to exit. This means that each close must be in a try-catch. Also watch such exceptions as null pointer exceptions, which the compiler does not force you to explicitly catch.
Put multiple close calls in the same try-catch. ( Method calls that release system resources (e.g., close) and might throw exceptions MUST be in their own try-catch block. For example:
```
   try {
      x.close();
      y.close();
   } catch ....
```
If x.close throws an exception, the socket resources for y will not be released. Similar problems occur when you put the close in the try-catch in which you do I/O. In both cases, a long-running server will soon exceed its resources.
When a communication thread gets an I/O exception, it should close both sockets. Some people only closed one, resulting in the program hanging.
Improper messaging when an exception occurs. Think about when you want to print a message in response to an exception and when you don't. Most exception messages should go to System.err not to System.out.
Improper order in catching exceptions. (You want to catch most specific exceptions first...at most one will be caught.)
Failed to close both sockets in a thread. (If your program behaved correctly, you got lucky.)

Writing style and presentation: Actually, the reports this semester were much better than in previous semesters!
- Spelling and grammar. (Yikkes! Use a grammar checker and pay attention to those green underlines, please!)
- Including completely irrelevant information in the introduction (to make the report look longer?)
- Changing style, typeface, number scheme or bullet style in the middle of the report. (Use Times-Roman 12-pt type so I don't go blind. Also, please single-space and hand in with a single staple in the upper left corner. If your report involved heavy deforestation, please staple individual sections and then use a big spring clip to put everything together.)
- Poor layout and organization. (Please follow the guidelines for the reports.)
- Presented the entire report in a single paragraph or in very long paragraphs. (Use paragraphs. If you have a single-spaced paragraph that is a third of a page or more, you probably need more paragraphs or to be more concise.)
- Inappropriate or lack of section titles. (Bold face the section titles and make the section titles correspond to what the section is about. Your goal is to convey information, not to challenge the reader's logic skills.)
- Excessive use of code in the report. (Some people pasted parts of their programs into the report. Use outlines, pseudocode or block diagrams to convey implementation details.)
- Excessive use of "it". (After 20 sentences that start with "it" on the same page, the reader won't know what it you mean. My personal style problem is starting too many sentences with "This is". Some of you also have this problem.)
- Gave separate architectural diagrams for the tunnel and proxy, but the diagrams were identical. If the diagrams are going to be the same, you should give one diagram and refer to it when discussing both items.
- Gave a confusing diagram. Diagrams are very useful and can greatly improve the clarity of the presentation, but a diagram that conveys the wrong idea is worse than no diagram. Ask yourself, what information you are trying to convey by the diagram. For example don't use the same style box to represent both a process and a port number, or the same type of arrow to represent a connection request and a thread.
- Didn't answer the questions. (The specification asked several questions that you were supposed to answer.)
Poor testing and presentation of results:
- Did not prove that the program correctly transmitted the files. (Although it might appear that your program is working because the browser displays a page, you must test that the program correctly transmitted the data. For the tunnel, you should have first tested between a modified TCPClient and TCPServer to make sure that the tunnel correctly transferred random files in both directions --- including binary files. I could tell from reading code that contained BufferedReader or readUTF or used String for the files, that this was not done and that the test cases had been carefully selected.)
- Did not adequately explain the tests. The typical approach was a short paragraph saying the program worked. A large log file was then included. (A good approach might be to organize your test results into a table with an annotation of what that test result was supposed to show and a column with page numbers of the output so that the reader can actually see the tests.)
- Random tests. (You should think about what you are trying to test for --- different types of web pages, different types of servers, different network connections, different times of day, etc. and clearly organize your tests.)
- Did not state conditions under which experiment was run (machines, times of day, etc.)
- Did not give an analysis of what happened, what you expected to happen and what actually did happen.
- Assumed everything was a String. (Clearly you didn't run tests with binary objects---or at least didn't mention it.)
Programming errors and bad style:
- Big or inconsistent indentation.
- Big loops - use helpers to remove complexity. For example, parsing the GET line should be done in a helper.
- Don't reinvent the wheel. (For example the URL class has everything you need to parse a URL.)
- Used String to concatenate characters in a loop --- which results in copying to a completely new String on each concatenation. (Use StringBuffer which is like String, but supports mutable strings.)
- Wrote completely different programs for the two directions on the proxy, when most of the code is the same. (A second constructor where you pass the parsed GET would be good here.)
- Assumed that what was read would not exceed some arbitrary, predetermined size. For example, some people assumed that the entire header would fit in a specified number of bytes or was less than a specified number of lines, say 50. (When I see a buffer of 400000 bytes stuck in the code, I immediately suspect that code isn't working right.)
- Not creating new classes to encapsulate complex operations, say logging or parsing. (As you become more experienced with OO programming, you will start to think in these terms, and your programs will greatly improve.)
- Wrote error messages to System.out instead of to System.err. While the distinction doesn't appear to matter much when you are testing, be aware that for actual servers its important. Servers generally run in the background and often have standard out redirected to the null device and standard error redirected to the system log. You should also be careful about what messages you output on exceptions that are thrown due to normal behavior --- such as EOF.... remember their messages have to go somewhere.
- Proposed a naming convention for the resource files in the report and then didn't follow it in the program.
- Numeric values for things like LF and CR. Define constants for them.
- Hard-coded buffer sizes into the program. (Use predefined constants with static final for buffer sizes if they have to be fixed size. Then you only have to change the values in one place and you know what the values represent.)
- Used port 80 all of the time for the proxy. (Port 80 should only be used as the default, if the URL does not include a port number.)
- The command-line arguments were not used or were used incorrectly.
- Using new outside of any constructor or method. For example initializing a field with:
```
      private Vector myList = new Vector();
```
  What happens if the new throws an exception?

Last revision: March 3, 2003 at 9:10 am by Kay A. Robbins

CS 5523 Operating Systems Laboratory 1 Post Mortem Proxy Monitor Spring 2003

CS 5523 Operating Systems
Laboratory 1 Post Mortem
Proxy Monitor Spring 2003