0

I have a singleton that keeps its current state inside of an enum. I want to be sure that this implementation is thread-safe, does not have serialization issues and is performant. It is being used for central configuration and to dynamically change the configuration at run-time. May anyone please point out any issues that you can find. I realize the synchronized blocks may slow the access to an internal enum value.

The set operation is not called very often maybe once a day, but the get operation is called 200-600 times an hour.

I have tried running this with different threads accessing the value and setting value in the enum.

import java.io.Serializable;

public class ProviderSingleton implements Serializable {

    private static final long serialVersionUID = -6827317560255793250L;
    private static volatile EmailProvider emailProvider;
    private static ProviderSingleton instance = new ProviderSingleton();

    private ProviderSingleton() {
        System.out.println("Singleton(): Initializing Instance");
        emailProvider = EmailProvider.SMTP; // the default
    }

    public static ProviderSingleton getInstance() {
        return instance;
    }

    public String getEmailProvider() {

        synchronized (ProviderSingleton.class) {
            return emailProvider.name().toString();
        }
    }

    public void setEmailProvider(String emailProviderValue) {

        synchronized (ProviderSingleton.class) {
            emailProvider = EmailProvider.valueOf(emailProviderValue);
        }

    }

    // This method is called immediately after an object of this class is
    // deserialized.This method returns the singleton instance.
    protected ProviderSingleton readResolve() {
        return getInstance();
    }

    enum EmailProvider {
        SMTP, POP3;
    }
}

I am hopping issues can be pointed out or improvements suggested with code examples.

Udara Abeythilake
  • 1,215
  • 1
  • 20
  • 31
CuriousProgrammer
  • 1
  • 1
    If all you do is return a string, why not save the string instead of returning `toString` each call? Do you expect that string to change between calls? – markspace Jul 24 '19 at 04:03
  • See Question, [*What is an efficient way to implement a singleton pattern in Java?*](https://stackoverflow.com/q/70689/642706) – Basil Bourque Jul 24 '19 at 04:17
  • @markspace I use the toString to make sure I receive a string not an enum, do you think this is unnecessary? – CuriousProgrammer Jul 24 '19 at 05:20
  • @BasilBourque I have looked at this, it does not answer my usec-ase. – CuriousProgrammer Jul 24 '19 at 05:22
  • Why? I don't see any need for this class to be a singleton in the first place. It doesn't do anything really except wrap the `Enum`, which is already a singleton. – user207421 Jul 24 '19 at 05:46
  • @user207421 please explain, the example I see all over the internet look like this `code` public enum MySingleton { INSTANCE; public void doSomething() { ... } public synchronized String getSomething() { return something; } private String something; } `code` there is only one value for the enum, it does not look it can have multiple states , I might be overthinking it but please elaborate. – CuriousProgrammer Jul 24 '19 at 06:15
  • 2
    You still haven't stated you need a singleton around a static value. Or, for that matter, why you think you need to serialize it. – user207421 Jul 24 '19 at 07:44
  • 1
    There is no point in declaring `emailProvider` as `volatile` *and* use `synchronized` for every access to it. You need only one of them. Further, `emailProvider.name()` returns a `String` and there is no point in calling `toString()` on a `String`. Then, the enforcement of the singleton property is not bullet-proof here, but said by others, an existence of multiple instances would not have any effect at all. – Holger Jul 25 '19 at 11:28

0 Answers0