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. 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.
- Should you RunFinalizersonExit? Different versions of Java seem to vary in how they treat finalizers. If you think that you might compile your code under versions earlier than Java 1.3, it might be a good idea to use this. You may then need to use the -deprecation switch during compilation.
- Keeping the data of the FileObjects in memory: Your FileObjectImpl should not keep the array of bytes in memory. Otherwise, as more objects are added to the directory, it will exceed the memory capacity of the machine. I observed a subtle bug in this regard. During serialization, most of you read the bytes from the file into a temporary array. If the array is a local variable, there is no problem. Some people made this array a field and did not explicitly set it to null after the read. Thus, once an object serialized once (by a remote client calling getFile) its data would be kept permanently in memory. Thus, the directory implementation would appear to have a memory leak. Some people also had this problem for the read method. One person did not have a disk file at all in the FileObject.
- Using the logical file name as the disk file name: Obviously some people missed my harangue in class about this. 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 dleete the old file afte the new one is in place.
- "Unauthorized" enhancements to an interface and related issues: Some people changed the FileObject and/or Directory interface to make their implementations easier. If you are given an interface --- this is what you have to work with --- anything else fails to meet the specs. Another mistake was to add additional public methods to FileObjectImpl (which was okay) but then to have DirectoryImpl call this special methods. The DirectoryImpl then will not work on any FileObject, just a special kind with the extra method. A related problem was not making the fields private. In the FileObjectImpl, for example, some people provided a constructor with no parameters that did not set the filename. They then directly set the file name later because it was public or protected. This stategy bypasses the whole nature of OO design. Make all of an object's fields private, UNLESS there is a specific documented reason for not doing so.
- Alphabetizing the list of file names: Some people didn't do it at all. Those that did used one of three approaches:
  - Find the position in the vector and insert the new name during create.
  - Put the element at the end and then sort the vector by either copying it into an array or using Collections.sort during create.
  - Put the element at the end and then sort the vector by either copying it into an array or using Collections.sort during getListing.
  Why is the middle alternative the worst of all worlds?
- 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.
Uncaught exceptions, badly caught exceptions, and bad exits:

Allowing bad client input to cause the server to exit: The client should NEVER be able to cause the server to exit. Someone actually explicitly called System.exit in the server, when the client gave an empty string as a file name.

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.

Throwing and catching the same exception: If you're going to catch an exception and then immediately rethrow it, you should have a reason for doing so --- like outputting an error message, performing clean-up, or providing an enhanced error message for the exception itself.

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 a RemoteException with an informative message. A RemoteException also has a constructor which allows you to insert the Exception that caused the RemoteException to be sent.

Made FileObject remote in part 2: This was not in the spec.
The update replaced the whole FileObject rather than just updating the data: Why isn't this equivalent to the specification?

Using readLine to read in the data file: The specification did not say this should work only for text files.
Assuming a fixed size limit for the file: The file did not have a size limit. People assumed limits of 200 bytes, 512 bytes, 1024 bytes, 2024 bytes (2024 bytes?) and 2048 bytes.

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.
- 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. It would be good to reread it now that you have some experience with these issues.)
- 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.
- Synchronizing FileObjectImpl. The FileObjectImpl should really be synchronized using reader/writer synchronization, althought 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).
- 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?
- 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?
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 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 better this round. There were some well-written reports. However a significant number of reports consisted mainly of a large general introduction about Java RMI that appeared to be condensed or lifted directly from various Java tutorials. The implementation section listed the methods to be implemented and the testing section consisted of a sentence saying that the program was tested and worked. 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.

Last revision: April 7, 2002 5:30 pm 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