Singleton and multithread

Question

Please, look at the classes below and tell me if the code below is thread safe. The point of my question is that one class whose static method and that method calls singleton instance's method. Also, the static method is invoked by Runnable instance. So I am asking you guys to see the codes - static method and it calls singleton's method in multi thread environment - is safe?

I will really appreciate if you answer my question.

import java.io.BufferedReader;
import java.io.FileReader;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.StringTokenizer;

public class SingletonCls {
    private static SingletonCls singletonInstance = null;

    private SingletonCls() {
    }

    public static SingletonCls getIntance() {
        if (SingletonCls.singletonInstance == null) {
            singletonInstance = new SingletonCls();
        }
        return SingletonCls.singletonInstance;
    }

    public List<Map<String, String>> call(String id) throws Exception {
        List<Map<String, String>> list = new ArrayList<Map<String, String>>();
        BufferedReader br = null;
        final String col = "col";
        try {
            br = new BufferedReader(new FileReader("test.txt"));
            String lineStr = null;
            while ((lineStr = br.readLine()) != null) {
                StringTokenizer st = new StringTokenizer(lineStr, ",");
                int colIdx = 1;

                if (lineStr.startsWith(id)) {
                    Map<String, String> map = new HashMap<String, String>();
                    while (st.hasMoreTokens()) {
                        String value = st.nextToken();
                        map.put(col + (colIdx++), value);
                    }
                    list.add(map);
                }
            }

        } finally {
            if (br != null) {
                br.close();
            }
        }
        return list;
    }
}


import java.io.IOException;
import java.util.List;
import java.util.Map;

public class TestSingleTonCaller {

    public static List<Map<String, String>> getData(String id) throws Exception {
        List<Map<String, String>> list = SingletonCls.getIntance().call(id);
        return list;
    }
}



import java.io.IOException;
import java.util.List;
import java.util.Map;

public class RunnableSingleTonExe implements Runnable {
    private final String id;

    public RunnableSingleTonExe(String inId) {
        this.id = inId;
    }

    public void run() {
        try {
            List<Map<String, String>> list = TestSingleTonCaller
                    .getData(this.id);
            System.out.println("thread id:" + this.id + "  list > "
                    + (list == null ? "" : list.toString()));
        } catch (IOException e) {
            Thread.currentThread().interrupt();
            e.printStackTrace();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Since you're not creating a singleton correctly, no. There's no guarantee you will only have one instance of `SingletonCls`. Use an `enum` in Java for singletons — Brian Roach, Jul 04 '13 at 15:57
`SingletonCls#getInstance` method is not thread safe. It would be better to initialize `singletonInstance` from the beginning instead of lazy loading it. — Luiggi Mendoza, Jul 04 '13 at 15:57
@Yoonyou Ryu - As Brain and Luiggi indicate, this is not threadsafe and there are better approaches. See http://stackoverflow.com/questions/3635396/pattern-for-lazy-thread-safe-singleton-instantiation-in-java for other ways to do this. — lreeder, Jul 04 '13 at 16:02
Thank you all n your kind answer. I also totally agree with you. I just made the codes to simulate what my forks made. And I can be sure that codes are not safe n good. Thanks again — Yoonyou Ryu, Jul 04 '13 at 22:27

fge · Answer 1 · 2013-07-04T16:12:18.510

It is not safe because this scenario can happen:

Thread 1                Thread 2
--------                --------
test instance != null
                        test instance != null
                        finds it is
finds it is
creates, assigns
                        creates, assigns
                        returns
returns

In essence, this is not a singleton anymore.

Note also that you cannot guarantee which created instances either thread will return since singletonInstance is not even volatile!

Easy fix, since your constructor does nothing:

private static final SingletonCLS INSTANCE = new SingletonCLS();

public static SingletonCLS getInstance() { return INSTANCE; }

Other possible solutions:

use an enum;
use a lazy initialization holder class.

Thanks for your answer. :) I also told my co-worker about such scenario but he didn't listen. But I can be sure that code have wrong. i am gonna tell him again. Thanks again. — Yoonyou Ryu, Jul 04 '13 at 22:32

score 0 · Answer 2 · answered Jul 04 '13 at 16:34

First of all I don't think you need a Singleton here. You have no Instance Variable in the singleton Class. The method could be static very Well. Therefore its Thread Safe in the Context that Singleton is not needed.

Second, Your Singleton is wrong (if it is required). Please refer Effective Java Item 71. try using (lazy initialization holder class idiom).

Second It may not be a good idea opening the same file again and again. better read and cache the data in memory and then try finding id. In this case you will need a SingleTon Object.

Thank you for your answer. N I also think lazy initialization is not necessary. I am gonna get my co-worker to fix it. — Yoonyou Ryu, Jul 04 '13 at 22:32

Mac · Answer 3 · 2013-07-04T17:01:33.967

Your getIntance() method is not thread-safe. And this could lead to creation of more than one object of SingletonCls as specified in above answers. To get the Singleton version of your class with lazy instantiation you should use following code:

public class SingletonCls 
{
    public static SingletonCls getInstance()
    {
        return LazyClass.getInstance();
    }
    private static class LazyClass
    {
        public static SingletonCls instance = new SingletonCls();
        public static SingletonCls getInstance()
        {
            return instance;
        }
    }
}

This relies on the fact that inner classes are not loaded until they are referenced. This way of creating Singleton class is called as Initialization on Demand Holder

Thank for that link of lazy initialization holder. I will let my co-worker know the link and use the singleton holder if he insist to use lazy initialization. Thanks again. — Yoonyou Ryu, Jul 04 '13 at 22:38

Singleton and multithread

3 Answers3