Java 1.4 singleton containing a mutable field

Question

Hi, I'm working on a legacy Java 1.4 project, and I have a factory that instantiates a csv file parser as a singleton.

In my csv file parser, however, I have a HashSet that will store objects created from each line of my CSV file. All that will be used by a web application, and users will be uploading CSV files, possibly concurrently.

Now my question is : what is the best way to prevent my list of objects to be modified by 2 users ?

So far, I'm doing the following :

final class MyParser {
    private File csvFile = null;
    private Set myObjects = Collections.synchronizedSet(new HashSet);

    public synchronized void setFile(File file) {
        this.csvFile = file;
    }

    public void parse()
        FileReader fr = null;
        try {
            fr = new FileReader(csvFile);
            synchronized(myObjects) {
                myObjects.clear();
                while(...) { // foreach line of my CSV, create a "MyObject"
                    myObjects.add(new MyObject(...));
                }
            }
        } catch (Exception e) {
          //...
        }
    }    
}    

Should I leave the lock only on the myObjects Set, or should I declare the whole parse() method as synchronized ?

Also, how should I synchronize - both - the setting of the csvFile and the parsing ? I feel like my actual design is broken because threads could modify the csv file several times while a possibly long parse process is running.

I hope I'm being clear enough, because myself am a bit confused on those multi-synchronization issues.

Thanks ;-)

Answer 1

I think there are multiple issues which are there in this code.

1. If this class is a singleton, this class should be stateless i.e no state should be present in this class. therefore having setter for the file itself is not the right thing to do. Pass the file object into parse method and let it work on the argument. This should fix your issue of synchronizing across various methods

2. Though your myObjects Set is private, I am assuming you are not passing this to any other calling classes. In case you are, always return clone of this set to avoid callers making changes to original set.

3. Synchronized on the object is good enough if all your set changes are within the synchronized block.

Answer 2

Use separate MyParser object for every parse request and you will not have to deal with concurrency (at least not in MyParser). Also, then will you be able to truly service multiple users at a time, not forcing them to wait or erasing the results of previous parsing jobs.

Answer 3

Basically you are assuming methods need to setFile first and then call parser. Let us consider this, t1 (with setFile XX) and t2 (with setFile YY) are coming at the same time and t2 set the file to be YY. Then t1 asks for parse() and starts getting records from YY. No amount of synchronised is going to solve this for you and the only way out is to have the parse method take a File parameter or remove the singleton constraint (so that each thread has its own file object). So use a

public void parse(File file) //and add synchronised if you want.

Answer 4

The singleton thing is mostly a red herring. Nothing to do with concurrency issues you are considering. As far as synchronisation goes, I think you are ok. Making the method synchronized will also work despite the fact that myObjects is static because it is a singleton.