CODEWITHSUNDEEP
javamultithreadingthread-safety
user5447339
user5447339

Reputation:

How to make factory thread safe?

I have a below factory which will get called from multiple threads. I want to know if it is thread safe or not.

public class ClientFactory {
  private Map<TypeName, Provider> holder = new HashMap<>();

  private static class Holder {
    private static final ClientFactory INSTANCE = new ClientFactory();
  }

  public static ClientFactory getInstance() {
    return Holder.INSTANCE;
  }

  private ClientFactory() {
    holder.put(TypeName.typeA, new DataProvider());
    holder.put(TypeName.typeB, new ProcessProvider());
  }

  public IProcess createNewType(TypeName typeName) {
    Provider provider = holder.get(typeName);
    if (provider == null) {
      throw new IllegalArgumentException("invalid typeName is passed");
    }
    IProcess process = new ProcessImpl(typeName.name(), provider);
    return process;
  }
}

In my above factory createNewType method will be called from multiple threads. I wanted to make it thread safe. Is it thread safe? I am not modifying my HashMap once it has been populated in the ClientFactory constructor. So I believe it is thread safe since the calls to get() on that HashMap will be thread safe.

If it is not thread safe then how can I make it thread safe in the most efficient way?

Upvotes: 1

Views: 2377

Answers (2)

Kedar Mhaswade
Kedar Mhaswade

Reputation: 4695

Short answer: You should declare holder to be final. Then, the so-called publication of the shared HashaMap is safe. After the publication all the threads can see it and since the contents of the map do not change, it is expected to behave like an immutable map and hence is thread-safe. The other suggestion to make the map unmodifiable is a good one too.

Some well-intentioned rants: Even though you seem to be worrying about thread safety of your factory, there are a few other non-threading related issues with it that you should fix (first).

  1. There is no need to use the Holder pattern in such a convoluted way. For instance, what's the need for an inner class just to hold the instance that you immediately construct?

  2. Rename holder to something more meaningful, e.g. typeToProvidersMap. Yes, it is more verbose, but makes the code (much more) readable.

  3. Is this is a factory of Clients or factory of Providers? If it is the latter, rename the class appropriately. Also, rename the createNewType() to createNewProvider() because creating a new provider is what the method seems to do.

Even if this is example code, it could still be exemplary.

By 1), I mean that following is better to begin with:

public class ClientFactory {
  private final Map<TypeName, Provider> typeToProvider = new HashMap<>();
  // constructing the singleton since the construction needn't be lazy
  private static final ClientFactory INSTANCE = new ClientFactory();

  public static ClientFactory getInstance() {
    return INSTANCE;
  }
// rest is omitted for brevity
}

The Holder pattern is to be used when the lazy initialization is desirable with a specific purpose in mind.

Upvotes: 2

n247s
n247s

Reputation: 1918

If your HashMap isn't modified, than the Factory class is Thread safe. Though the problems may start when using the Objects in the map (as they are not Thread safe, even if the Map implementation would be Thread-safe).

So make sure the Provider implementations are Thread safe, or lock the whole Provider instance on acces (which may notbe the best solution).

Upvotes: -1

Related Questions