1

I'm running a long running web service via Jetty/CometD, and I'm using the Redisson library for connecting to redis. I'm using a Singleton pattern for getting my RedissonClient/connection and I am not sure if that is the best way to go.

Class looks like this:

public class RedisClient {
    // singleton instance of our RedisonClient/connection
    private static RedissonClient _redissonInstance;
    public static String REDIS_HOST = "my.redishost.com:6379";


    private static RedissonClient setupRedis() {
        org.redisson.Config config = new org.redisson.Config();

        config.useSingleServer()
                .setAddress(REDIS_HOST)
                .setConnectionPoolSize(200);

        return Redisson.create(config);
    }

    public static RedissonClient getRedis() {
        if (_redissonInstance == null) {
            _redissonInstance = setupRedis();
        }
        return _redissonInstance;
    }

    public static void setRedisHost(String redisHost) {
        _logger.warn("Setting REDIS_HOST to: " + redisHost);
        REDIS_HOST = redisHost;
    }
}
Joakim Erdfelt
  • 46,896
  • 7
  • 86
  • 136
giantNinja
  • 191
  • 2
  • 9
  • 1
    I can't think of any use case where Singleton would be a good idea. – duffymo Mar 02 '16 at 18:21
  • Why are singletons a bad idea in Java? Seems like the more I read, the more it seems they aren't recommended (still learning Java btw) – giantNinja Mar 02 '16 at 22:08
  • They're a bad idea in any language. That's why they aren't recommended. Google has written tools to detect them for elimination: http://googlecode.blogspot.com/2007/07/google-singleton-detector-released.html – duffymo Mar 03 '16 at 02:23
  • Agree in general with @nikdeapen that `public static final` is a bad choice for an instance holding so much of resources (TCP connections, Threads, ...). Any instance held mainly in a `static final` cannot be managed in a sensible way. Singletons in Spring, CDI or EJB are way better since you get a bean management to initialize and destroy the instance. Using a `static final` requires you to either close the instance yourself or add a shutdown hook. – mp911de Mar 03 '16 at 09:36
  • @mp911de I see you're point, thanks! Any chance you know how to set that up in a Jetty Servlet? I've got my MySql datasource setup as a JDNI resource, but can't seem to find a way to setup a resource for redis – giantNinja Mar 03 '16 at 15:34
  • Depends on how you have set it up. You can start a Jetty from a Spring Boot application for example and all the dependency stuff is managed outside of Jetty. – mp911de Mar 03 '16 at 16:06

1 Answers1

4

I would say that this is a bad idea. I don't think singletons are a good idea in general but even so this is not a good way to do it. Your code is not thread-safe and it seems as if you want to support multiple hosts.

If you really don't want to pass your redis client around to every component and your host is not going to change and want something quick and dirty try this:

public class Redis {
    public static final RedissonClient CLIENT;
    static {
        Config config = new Config();
        config.useSingleServer()
            .setAddress("my.redishost.com:6379")
            .setConnectionPoolSize(200);
        CLIENT = Redisson.create(config);
    }
}

This has the benefit of being thread-safe without having any synchronization when getting the reference.

nikdeapen
  • 1,581
  • 3
  • 15
  • 27
  • It's not so much that I want to support multiple hosts, but I'm setting the host from a maven profile property -> web.xml init-param which I get when the servlet is initialized and was setting the host for redis based on that. I have two redis hosts, one for development, and one for production, and I don't want to use the production redis host when I'm testing. Not sure the best way to go about that, but thanks for the reply and pointing out the thread safety issue – giantNinja Mar 02 '16 at 22:06