Thread synchronization for Static factory method

[Siva Masilamani]

Hi

I am trying to understand if I should synchronize the below factory method.I tested the code with three sample thread and I am always getting the correct object for each thread(meaning Thread 1 always shows TYPE1,thread2 always show TYPE2 and thread3 always shows TYPE3 but I am still thinking that this may not be the case always unless I synchronize the factory method else it will not be thread safe meaning thread 1 could get type 2 or type 3 also.

Also another option is moving the static field b inside method so that it will be a local variable.

Please correct me If am wrong.

Thanks in advance.

public class TestThread {

	public TestThread() {
		// TODO Auto-generated constructor stub
	}

	public static void main(String[] args) {
		// TODO Auto-generated method stub
		Thread t1 = new Thread("First") {
			public void run() {
				try {
					for (int i = 0; i <= 100; i++) {
						sleep(200);
						System.out.println(getName() + "->"
								+ Factory.getObject("TYPE1"));

					}
				} catch (Exception e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}
			}
		};
		Thread t2 = new Thread("Second") {
			public void run() {
				try {
					for (int i = 0; i <= 100; i++) {
						sleep(200);
						System.out.println(getName() + "->"
								+ Factory.getObject("TYPE2"));
					}
				} catch (Exception e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}
			}
		};
		Thread t3 = new Thread("Third") {
			public void run() {
				try {
					for (int i = 0; i <= 100; i++) {
						sleep(200);
						System.out.println(getName() + "->"
								+ Factory.getObject("TYPE3"));
					}
				} catch (Exception e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}
			}
		};
		t1.start();
		t2.start();
		t3.start();

	}

}

Factory Class:

public class rFactory {
	private static Map binders = new HashMap();
	static {
		binders.put("TYPE1", TYPE1);
		binders.put("TYPE2", TYPE2);
		binders.put("TYPE3", TYPE3);
	}
	static String b;

	public static String getObject(String schema)
			throws BusinessException {
		b = (String) binders.get(schema);

		return b;
	}
}

[Jayesh A Lalwani]

If you want to reproduce the error, you will need more than a 100 iterations in the loop. It could be that 100 loops run so fast that the first thread is done before the second starts

Making b into a local variable is a much better option. Each thread will get it's own variable.

[Siva Masilamani]

Thanks for your input but I was able to prove that this is not threadsafe (atleast in this case) by adding Thread.sleep(200) in the Facotry method and I see that thread started getting wrong object.

[Mike Simmons]

Excellent. But Jayesh's other point stands. Your class field b is completely unnecessary and should be removed; it's the source of the bug you're observing. I would also recommend making the binders variable final. That's not strictly necessary for a static field, but good practice (and prevents anyone else from changing it without realizing it's supposed to be constant). If the field were non-static, then making it final would be necessary in order to access it in a thread-safe manner without synchronization.