synchronizing io operation in java on a string method argument?

Question

Basically, I have a class with 2 methods: one to serialize an object into an XML file and another to read an object from XML. Here is an example of synchronized part from the method that restores an object:

    public T restore(String from) throws Exception {
     // variables declaration
     synchronized (from) {
      try {
       decoder = new XMLDecoder(new BufferedInputStream(
         new FileInputStream(from)));
       restoredItem = decoder.readObject();
       decoder.close();
      } catch (Exception e) {
       logger.warning("file not found or smth: " + from);
       throw new Exception(e);
      }
     }
    // try to cast it
    }

A similar approach is taken when serializing an object. Now, when I create a unit test which in turn creates 10 threads with each thread trying to serialize and instantly read either a Boolean or a String it would fail showing that ClassCastExceptions occur. This makes me think that I get serialization wrong (everything's ok in a single-threaded environment). If you've stayed with me down to this point :), here are the 2 issues I need your help on:

1. does it make sense to synchronize on a string argument passed to method (considering there's a string pool in java)? BTW, I've tried synchronizing on the XMLSerializer class itself with result staying the same.
2. how do i correctly synchronize io operations on a single file?

Answer 1

Given that the some parts of your code are missing, my bet is that the problem lies with synchronizing on a string. You cannot freely assume that strings are pooled (which would break your synchronization scheme).

The best approach would be to add a map that will associate a key (string) with its actual synchronization object.

Other than that, I would suggest to play with the multi-threaded test to see what make it fail. For example, if you make all threads only store string values (rather than strings or beooleans), does the test still fail?

Answer 2

There are a number of problems with this approach.

1. Unless you've called String.intern then your from string is probably not the same from as the other one you are calling. Relying on the behaviour of the internal java string cache is not very robust.

2. you aren't properly disposing of your XMLDecoder in a finally block, any exception thrown during that call will leak the file description associated with that FileInputStream.

3. You don't need to wrap e in another Exception(e), you can just throw e as you have declared the enclosing method also throws Exception

4. Catching/Throwing exception is a code smell. Yes, it is a super class of IOException, and whatever XML decoding exception might be thrown, but its also a superclass of a bunch of other things you probably didn't want to catch, NullPointerException for instance.

To answer your question, how can you serialize access to a shared file to ensure its not being used by more than one thread, is tricky. FileChannel.lock() doesn't work inside the JVM, they just lock the file from modification by other processes in the machine.

My approach would be to strip any locking out of this class and wrap it in something that is aware of the threading issues of your code.

I'd also not pass a String as the filename, but a File, which gives you the ability to use File.createTempFile(2) to create opaque filenames between the thing writing xml and the thing reading xml.

Finally, do you want to synchronise access to a shared file, or fail when you detect multiple access to the same file?

Answer 3

A String does not good make a good mutex, but can be used to create one: Java: synchronizing on an ID.

Answer 4

1. Yes, it's OK to synchronize on a String, however you'd need to synchronize on the string.intern() in order to always get the same Object

StringBuffer sb = new StringBuffer(); sb.append("a").append("b");
String a = new String(sb.toString());
String b = new String(sb.toString());
a == b; //false
a.equals(b); //true
a.intern() == b.intern(); //true

Since you want to synchronize on the same monitor, you'd want the intern().

2. You'd probably not want to synchronize on a String since it may synchronized on somewhere else, inside your code, in 3rd party or in the JRE. What I'd do, if I wanted to stay with synchronize, is create an ID class (which may hold only String), override equals() and hashcode() to match, put it in a WeakHashMap with both as key and value (see IdentityHashMap for idea why) and use only what I .get() from the map (sync map{ syncKey = map.get(new ID(from)); if syncKey==null create and put new key} sync{syncKey}).

3. Then again, I'd ditch synchronize all together and use the java.util.concurrent.locks.Lock instead, in the same setup as above only with the lock attached to the ID.

Answer 5

see here

http://java.pakcarid.com/default.aspx?sub=24&Sls=24