2

My FileManager exposes a write method which contains a synchronized block to prevent concurrency issues:

class FileManager{
    Object lock = new Object();

    public void write() {
       synchronized (lock) {
          String id = nextId();
          write(id);
       }
    }
}

However, if multiple instances of FileManager exist, their write methods can still be executed concurrently. Would making FileManager a singleton be a good fix to this possible concurrency issue? Or should I use syncrhonized keyword in another way?

ab11
  • 19,770
  • 42
  • 120
  • 207
  • 2
    If you don't want two `FileManager` instances to being able to run `write` simultaneously, you _could_ make it a singleton. Or you could just make the `lock` field static. Then your `FileManager`s would all be syncing on the same object. – khelwood Jun 13 '17 at 12:56
  • Any suggestion as to which is more desirable? – ab11 Jun 13 '17 at 12:57

3 Answers3

2

There are couple of ways you can do it:

  • Make it a singleton, as you are also suggesting, but this is the least desired way to go for (I would not at all recommend it), because making class singleton just for synchronization is an over kill, moreover singeltons are now discouraged.
  • Make you lock object as static i.e. static Object lock = new Object();, in this way you will always be locking on the class level lock object.
  • Make the write method as static and synchronized i.e. public static synchronized void write() {, in this way again you would be using the class level lock object but difference would be that this time you would be using the class level lock of FileManager and not your lock object.

Some other points to consider:

  • Whether you choose class level locking or object level locking, you are directly exposing your lock (especially true for class level locking), this basically means that some client of your API could hold the lock and all your methods which are relying on the object/class level lock will be blocked. For example, in case of class level locking, some client of your API could do synchronized(FileManager.class){ and all your static + synchronized methods will have to wait until that client has released the lock, now if that client is doing some intensive operation then it is performance penalty.
  • If you use some member variable for locking like this private static final Object lock = new Object(); then one important thing to note is that make it private otherwise it will still expose your lock. Otherwise it doesn't suffer from the issue which I have mentioned above. But only other thing with this approach is that - (1) you cannot use this at method level, you have use this lock within method by enclosing code blocks in the synchronized block. (2.) If suppose client of your API want to synchronize with some of your synchronized method which is using this lock then they cannot. But there are more deeper things, now a days typically this is the way locking would happen.
  • If you are using recent versions of Java then there are APIs like ReentrantLock and ReadWriteLock which provides more granular support for locking.

Choose which ever suits best your requirement.

In general comment, using synchronized or private static final Object lock = new Object(); for locking is the old way of locking, with the new concurrency API provided by Java, you should be using APIs like ReentrantLock and ReadWriteLock for implementing your synchronization mechanism.

hagrawal7777
  • 14,103
  • 5
  • 40
  • 70
  • If I choose the 3rd, would it lock all static FileManager methods? If I had `public static String read()` (not synchronized) would it be unable to execute during execute of the synchronized `write` method? If so, this would seem like a slight advantage of the 2nd option? – ab11 Jun 13 '17 at 13:07
  • If your `read()` is not synchronized then there is no issues, but if your `read()` or any other method is synchronized and static then all such static and synchronized methods will wait for each other. Just wait I am updating my answer to mention few comments about the same. – hagrawal7777 Jun 13 '17 at 13:09
  • @ab11 - I think the locking would be confined to other `synchronized` static methods in your `FileManager` class. So any non `synchronized` static method would not be impacted. – Krishnan Mahadevan Jun 13 '17 at 13:09
0

Whether or not you use a singleton pattern depends on whether or not individual FileManager instances have their own state or not. If all instances would essentially share the same state then yes, the singleton pattern would be a good choice here. If on the other hand they can have different state but you still need to synchronize method access across instances then you could synchronize on a static class member, e.g.:

class FileManager {
    static final Object lock = new Object();
    public void write() {
       synchronized (lock) {
            ...
       }
    }
}
Julian Goacher
  • 567
  • 2
  • 6
0

However, if multiple instances of FileManager exist, their write methods can still be executed concurrently?

Yes. They can run in parallel since lock exists at instance level i.e at object level and not at class level.

Would making FileManager a singleton be a good fix to this possible concurrency issue?

No. Making a class singleton if it had synchronized blocks is not necessary. But if you really need only instance of FileManager as per your design, they you can go for Singleton with static lock.

should I use syncrhonized keyword in another way?

You can use Lock and ReentrantLock as other alternatives.

Refer to below posts for more details:

What does 'synchronized' mean?

Avoid synchronized(this) in Java?

Synchronization vs Lock

Ravindra babu
  • 37,698
  • 11
  • 250
  • 211
  • This seems to be in conflict hagrawal's answer and my understanding. If `lock` is a non-static local variable, how would calls to different instances of `FileManager.write()` block each other? – ab11 Jun 15 '17 at 15:25
  • Small distraction from your question. Corrected it now. – Ravindra babu Jun 15 '17 at 15:30