CS 5523 Operating Systems
Laboratory 2 Post Mortem
A Remote Directory Service

Programming errors:
- Improper version numbers: The write method of FileObject should increment the version number. If you call write as part of readObject, the version number of the object that the remote client receives will be one greater than it should be. The solution is either to decrement in readObject or (maybe better) have both write and readObject call the same helper that does the actually writing but doesn't do the version increment. Some people set the version number to 0 in readObject. Why is this incorrect from the viewpoint a directory repository?
- Incorrect or missing finalize for FileObjectImpl: It is a good idea to delete any temporary or unneed disk files as soon as possible, but you still needed to have a finalize method that explicitly removes the disk file when the FileObject is garbage collected. If you have to remove multiple files, be sure that your finalize method catches necessary exceptions so that it doesn't abort before finishing its cleanup. There is no guarantee that the finalize method will ever be called. You should also call deleteOnExit for each temporary file that you create. Note that deleteOnExit only deletes the file when the entire program exits. This method guarantees that when the server exits, all of the disk files associated with temporary files will be deleted. You can encourage the Java Virtual Machine to run the finalizers of objects that are pending deletion by calling System.runFinalization.
- Hard-coding a maximum array size for the FileObject: You should make no assumptions about the size. You can always find out before allocated the array.
- Using a Vector of Byte objects instead of an array of byte values: The first implementation makes a very inefficient FileObject.
- Using the logical file name as the disk file name: When a file is updated, you must be sure that the original contents are preserved, if an exception is thrown during the update. The most straightforward way to do this is have the name that it is stored under different from the file's logical name. Most implementations that used rename either failed to preserve file contents under some circumstances or left an extra file afterwards. Remember rename can throw an exception. Using rename is completely unnecessary --- just keep the logical and physical name separate. Be sure to delete the old file after the new one is in place. Hardcoding a file name for the temporary file works as long as the FileObject is synchronized.
- Confusing a non-existent file with one that has no data: A non-existent file has no FileObject. A file with no data has a FileObject, but the disk file with the data either hasn't been created or is empty.
- Thinking that read of a file returns as many bytes as you have requested: Either use readFully or put your read in a loop. (This becomes a particular issue in the serialization functions, which appear to have a 1024 block size.)
- Returning an internal pointer to an object's data: Several people implemented the read method of the FileObject by simply returning a pointer to the internal data structure. Similarly others implemented the write method by setting the internal array to the array passed as a parameter rather than copying. This is an object-oriented programming disaster.
- Not maintaining consistent data structures: For example, some people removed an object from the Hashtable but not from the Vector. Others implemented update by creating a new FileObject and replacing the old one. This has several problems. The version numbers are off and because FileObject is a remote reference. The clients who had references before the replacement will be left in an inconsistent state.
- Corba encoding: don't put in sequence of a fixed length, if the sequence doesn't have a length limit.
- Used a byte value of -1 to mark the end of the data: Bytes can have any values.
- Improper close or improperly placed close: Several people for got to close their input or output streams in the FileObject read or write method. Also, some people put the close in the same try-catch with the read or write.
- Alphabetizing the list of file names: Some people didn't do it at all. One or two others implemented their own O(N^2) sort. Recopying and sorting on each getListing is likely to be inefficient. The Arrays class has an efficient static sort method.
- Other Inefficiencies: Several people used a binary search or linear search on the vector instead of the hashtable for finding whether or not the object was in the directory. Hashtable access should be O(1), while binary search can be even worse than a linear search if the Vector class is implemented as a linked list. The TreeSet is a nice class to look at. Several people did an extra copy on the getListing method. Also, for copying, consider using the arraycopy method in the System class.
Uncaught exceptions, badly caught exceptions, and bad exits:

Bad messages on thrown exceptions: Thrown exceptions are a method's vehicle for communicating errors with the caller. You should make sure that the error messages in the throw are informative. For example, on getFile you should not use File not found as the error message. Instead, you should say something like: File Lecture22.pdf not found. Remote exceptions in particular should be very informative, but you should be careful not to put something in the error message that the client should not know. Often you have more specific information about the error than is contained in the system's exception message when an exception occurs. It is often useful to catch the exception and rethrow it with a more detailed error message.

By-passing the exception mechanism: If an interface specifies that Exception is thrown, the method should throw an exception on an error. Some people bypassed the exception mechanism, for example, by catching all exceptions on read in FileObject and then returning null.

Generating incorrect remote exceptions: The DirectoryImpl can generate exceptions from many sources. In general, these exceptions should be caught and appropriate action should be taken. If the exception is something that the client needs to know about, then the method should throw the exception associated with the Corba interface.

Synchronization errors and serialization errors:
- Not saving the data on disk during readObject: Some people didn't save the data on disk so the object wasn't properly reconstituted.
- Not using correct data to reconstitute object during readObject: Some people actually had code in readObject that prompted the user to enter the data for the file. The data was supposed to come from the input stream.
- Changing the prototype of readObject or writeObject: for example, making it public. Your test programs should create an ObjectInputStream and ObjectOutputStream objects and call readObject and writeObject to test the serialization.
- Not using synchronization objects: Hard-coding synchronization directly into DirectoryImpl instead of creating a separate synchronization class is a very bad strategy that is nearly impossible to test adequately. You should create a separate synchronization class, say RWSync to enforce reader/writer synchronization. This class might have methods like startRead, endRead, startWrite and endWrite. (These aren't the only possible set of methods, but we'll assume these for the discussion.) The RWSync class can be tested in isolation so that you can be fairly confident that it works, before using it in the remote system. In fact, if you wanted to do a really nice job, you could define RWSync as an interface. You could then implement particular reader/writer policy such as strong reader in a class that implements RWSync. You can then change policies without changing the remote repository at all. Furthermore, you can use RWSync to synchronize both FileObject and Directory objects.
- Lock releasing and exceptions: Many people made errors in releasing locks. For example in the following code:
```
      mylock.startWrite();
        do some stuff that might throw an exception
      mylock.endWrite();
```
  the write lock will not be released if the exception occurs. There are two approaches to handling this problem:
  1. Release the lock in the catch and then rethrow the exception.
  2. Release the lock in a finally clause.
  The latter approach is probably safer since many people who used approach (1) did not actually catch all of the possible exceptions that could occur within that block. The finally clause is always executed regardless of whether or not an exception occurs and whether or not it is caught. (The Core Java Book Vol. I, Chapter 11 has a very nice discussion about exception handling, which would be good to reread now.)
- Serializing synchronization objects: If you synchronized FileObjectImpl using a synchronization object such as RWSync, you should be careful not to serialize it, because that will usually cause a deadlock when you unserialize. You can prevent a field from being serialized by including the keyword transient in its declaration.
- Synchronizing references to objects: The following code does not protect the Vector object myVector:
```
       Vector temp = null;
       mylock.startRead();
           temp = myVector;
       mylock.endRead();
       return temp;
```
  The variable temp is a reference to myVector, not an actual copy of myVector. This presents particular difficulties when temp is being returned in a remote call, as the return statement must serialize temp. It could be in the middle of serialization when another thread adds something to the vector. The correct approach is to clone the myVector object inside of the synchronization construct.
- Logging and synchronization: If you chose to log the operation of the server (which was not required, but a good idea), the methods of the printWriter and similar classes are not synchronized by default. You can overcome this problem by writing your own printWriterSynchronized class that extends printWriter and simply calls the super class method of the same name from a synchronized method.
- Serializing synchronization: Most people did not attempt to serialize a synchronized object. However, if you did, you need to declare your synchronization object to be transient so that it will not be serialized. Otherwise unserialization can cause a deadlock.
- The update method of DirectoryObject is a reader: Some people were confused because the update method called the write method of FileObject. However, as far as DirectoryObject synchronization goes, the update is a reader. In fact, the call to write should be outside of the synchronization completely.
- Improperly synchronizing FileObjectImpl: The FileObjectImpl should really be synchronized using reader/writer synchronization, although it wasn't required in this assignment. The file name never changes, once the object is created, so it doesn't have to be synchronized. However the version number and the size can change, so getSize and getVersion must both be treated as readers. The writeObject requires reader synchronization while the readObject requires writer synchronization.
- Synchronization within synchronization: Be careful not to nest synchronization calls as this can easily lead to deadlock. For example, if your write in FileObject is synchronized as a writer, you should be careful not to call write from another object that is also synchronized (such as readObject).
- Unpaired start and stop synchronization: One person called the stopWrite in both the catch and the finally clause. Similarly with the stopRead.
- Using synchronized and R/W monitor in the DirectoryImpl: If every method in DirectoryImpl is synchronized, the R/W will have no affect except to deadlock the system. Be sure you understand why.
- Using unrelated reader and writer locks: Some people used completely separate synchronization for readers and for writers. Why doesn't this work?
- Synchronizing the vector and hash table separately: You want the data in these two structures to be consistent so they should be synchronized together. What about backout if an operation fails on one?
- Race conditions due to test-and-set type operations: For example, suppose you use a Hashtable (which is synchronized), to test whether the FileObject is in the table before using reader-writer locks to actually insert the new FileObject. This code has a race condition because another caller could add the same FileObject to the table between the time when you tested for the object and when you acquired the writer lock. Why are the newer collections classes, such as HashMap, not synchronized?
- Using reader/writer locks with the synchronized keyword: Someone used reader/write locks for the read and write methods of FileObject, but the synchronizedkeyword for the getVersion.
- Holding a reader lock in update while writing a FileObject: Why doesn't this make sense?
- Incorrect synchronization of getListing: The vector needs to be cloned. Why?
- Holding locks when you don't need to: For example if you are doing some printing or sorting, you need to think carefully about whether you really want to prevent all others from accessing the directory while you are doing this.
Testing and presentation of results:
- Documenting test conditions for individual objects: I would have liked to have seen an analysis of the type of exceptions and conditions that each method was tested under, keyed to the test code that tested it. For example, "I wrote code to test all the methods" is really not an adequate testing strategy. You might include a main method for each object such as FileObjectImpl with the test code for testing the class in isolation. You can document your tests in the output of this main. If you make any changes to FileObjectImpl later, you can just rerun FileObjectImpl using its main.
- Making sure temporary files were gone: You should have stated explicitly that you checked to make sure that the temporary files were gone after the server exited. A few people mentioned that it took a while for the garbage collection to delete temporary files after the remove method of Directory was called. If you didn't mention it, I might wonder if you actually looked.
- Checking correct serialization: You should have stated explicitly that you checked that your serialized objects agreed with the original objects after serialization.
- Testing the reader/writer class before using it: Very few people showed any indication that they tested their synchronization class separately before incorporating it into the overall project. You can devise very simple tests of the reader writer in a threaded program, completely separate from the remote directory.
- Logging results: Someone (not this semester) provided an alternative constructor for DirectoryImpl with the name of a log file. If that constructor was used, then the DirectoryImpl logged its transactions to the indicated file. The logging used PrintWriter which is a class that catches all of its exceptions, particularly useful for logging purposes. This logging produced a very nice record of test results because it was actually built in to the class.
Writing style and presentation:
- Spelling, layout and typography were reasonable: There were some well-written reports. In general, people avoided copying large amounts of introductory material from tutorials and stuck to the point. (Thank you!) I look at the introduction to see if you understand what problem you are trying to solve and why it is significant. After that I am mainly interested in your testing, results and conclusions sections. I want to know what kinds of issues you ran in to and how you solved them, what you learned from the program. I would also like to see a convincing test plan.
- Testing descriptions were very weak:
- Should give more complete description of testing conditions: Should mention that you are compiling your idl with oldImplBase.
Last revision: April 21, 2002 5:30 am by Kay A. Robbins. This material may be used for educational purposes provided that the source is credited.

CS 5523 Operating Systems Laboratory 2 Post Mortem A Remote Directory Service

CS 5523 Operating Systems
Laboratory 2 Post Mortem
A Remote Directory Service