Java synchronisation poll

Question

Here's some code I saw once. Can you see what's wrong with it?

[updated]

public class ResourceManager1
{
    private final String mutex = "";
    Object resource = null;

    public Object getResource()
    {
     synchronized (mutex)
     {
      if (resource == null)
      {
       resource = new Object();
      }
     }

     return resource;
    }
}

public class ResourceManager2
{
    private final String mutex = "";
    Object resource = null;

    public Object getResource()
    {
     synchronized (mutex)
     {
      if (resource == null)
      {
       resource = new Object();
      }
     }

     return resource;
    }
}

Answer 1

mutex is not final and resource is not private.

Besides, you need a getResource method that returns resource, but I suppose this is just a typo.

Answer 2

Never synchronize on strings, particularly string literals which are interned. You've basically just got a single lock.

In general, never synchronize on any reference that might be visible outside your class (including "this") unless the purpose of the external visibility is precisely for locking purposes. I usually use a private final variable created solely for the purpose of locking.

Answer 3

You are using the same String as a mutex for both classes and hence only one of the synchronized blocks can be used at a time, which does not seem to be the intention of the code.