What's wrong with this code?

6

My college teacher gave us this code and was asked what's wrong:

public class Teste {
    private static Teste INSTANCE = null;

    public static Teste getInstance()
    {
        if ( INSTANCE == null )
        {
            INSTANCE = new Teste();
        }
        return INSTANCE;
    }

    private Teste() {
    }
}

I came to the conclusion was that:

  • Testing will never be created.
  • The getInstance will return "Garbage".
  • As the constructor is private it will never be called.
  • More than one test instance can be created
  • Would that be?

        
    asked by anonymous 14.02.2018 / 15:24

    1 answer

    6

    The code problem is that if there is more than one concurrent call on first access to the getInstance() method, it can create two Test instances.

    The basic solution to this is to synchronize the method:

    public static synchronized Teste getInstance()
    {
        if ( INSTANCE == null )
        {
            INSTANCE = new Teste();
        }
        return INSTANCE;
    }
    

    The problem with this approach is that all calls will be subject to deadlocks, making overall program execution slower. Imagine such a method on a multi-user application server accessing the system? It's terrible.

    A better solution would be a synchronized block within IF :

    public static Teste getInstance()
    {
        if ( INSTANCE == null ) {
          synchronized (Teste.class) {
            INSTANCE = new Teste();
          }
        }
        return INSTANCE;
    }
    

    This solves the problem of synchronization in all accesses, but it is a "naive" solution, because in fact we return to the initial problem. Since IF is not synchronized, two threads different can enter IF at the same time and, even with synchronization, they will return different instances when INSTANCE is null .

    So the "pure" solution to the singleton pattern would be to add a double check, like this:

    public static Teste getInstance()
    {
        if ( INSTANCE == null ) {
          synchronized (Teste.class) {
            if ( INSTANCE == null ) {
              INSTANCE = new Teste();
            }
          }
        }
        return INSTANCE;
    }
    

    With this last approach, we guarantee that there will be no performance loss because of unnecessary synchronization of the entire method.

    In addition, we guarantee that even if two calls competing with the method enter within the first IF , then we have a new synchronized check that guarantees a single Test instance.

    So, in the worst case, if there are two or more concurrent calls at the first access to getInstance (when INSTANCE is still null ), only these first calls will be synchronized, after the first assignment of INSTANCE , no later call will be synchronized.

    A complete reading of this you find in

      

    Head First Design Patterns.

    I recommend you search for Singleton

    Note: The private constructor prevents any other class from inadvertently or arbitrarily creating an undesired Test instance.

        
    14.02.2018 / 15:36