how to make this code thread safer?
in [Java Help]
|
From: www on 19 Jul 2010 12:37 The code below is completely fine if running on single-thread environment. But it could break if running on multi-thread environment. I have added my analysis as comments in the code to show my understanding why it is not thread safe. My real code, with a pattern like the code shown here, indeed breaks once in a while with Null Pointer Exception in multi thread run. Do you think my analysis is correct? Can you help me on making this code thread safer? I hope to keep the same pattern(having two individual methods), even though it looks like it is better to put every code inside doThis() method, then it will be thread safe. Thank you very much. public class MyClass { private Person tim = new Person("Tim"); private Person tom = new Person("Tom"); public void doThis { this.checkSomething(); if(tim != null) //thread "A" is running "doThis()" and tim is not null at this moment { ... //other code tim.doA(); //tim is NULL all of sudden, and NPE is thrown. I think it is due to another thread running checkSomething() tim.doB(); tim.doC(); } } public void checkSomething() //thread "B" is running checkSomething() { if(tim.isAbsent()) { tim = null; } if(tom.isAbsent()) { tom = null; } } }
From: Peter Duniho on 19 Jul 2010 13:08 www wrote: > The code below is completely fine if running on single-thread > environment. But it could break if running on multi-thread environment. > I have added my analysis as comments in the code to show my > understanding why it is not thread safe. My real code, with a pattern > like the code shown here, indeed breaks once in a while with Null > Pointer Exception in multi thread run. Do you think my analysis is > correct? Can you help me on making this code thread safer? The simplest fix is to just add "synchronized" to the method declarations. This will cause each method to acquire the instance's monitor during their execution, ensuring that only one thread can be executing either method at a time. Another alternative would be to use the "synchronized" statement to the code inside each method. Assuming the "isAbsent()" method isn't affected by the "other code" or the "doA()", "doB()", or "doC()" methods, that could look like this: public class MyClass { private Person tim = new Person("Tim"); private Person tom = new Person("Tom"); private final Object lock = new Object(); public void doThis { this.checkSomething(); synchronized (lock) { if(tim != null) //thread "A" is running "doThis()" and tim is not null at this moment { ... //other code tim.doA(); //tim is NULL all of sudden, and NPE is thrown. I think it is due to another thread running checkSomething() tim.doB(); tim.doC(); } } } public void checkSomething() //thread "B" is running checkSomething() { if(tim.isAbsent()) { synchronized (lock) { tim = null; } } if(tom.isAbsent()) { tom = null; } } } That would have a similar effect, but would only synchronize in thread "B" when the instance field actually has to be changed. It would still ensure that the field cannot change between the time thread "A" checks for the "null" value and when it tries to use it, but has the potential for being more efficient because in theory, less contention would occur ("contention" being when two different threads actually do need to acquire the same lock at the same time). Note the use of a dedicated object reference for synchronization. In general, it is poor practice to use the "this" reference for synchronization, though that's what declaring a method as "synchronized" does. It's not the end of the world to use "this", but doing so "leaks" some of your implementation, and introduces the possibility of some other code not related to your own taking the same monitor, increasing contention as well as the complexity of the locking scenarios. Those are the simplest techniques available in Java, and for the example you've given are probably the most appropriate anyway. As you learn more about concurrent programming, you'll probably want to look at the other synchronization features in Java, found mainly in the java.util.concurrent package. Pete
From: Jim Janney on 19 Jul 2010 13:10 www <www(a)nospam.com> writes: > The code below is completely fine if running on single-thread > environment. But it could break if running on multi-thread > environment. I have added my analysis as comments in the code to show > my understanding why it is not thread safe. My real code, with a > pattern like the code shown here, indeed breaks once in a while with > Null Pointer Exception in multi thread run. Do you think my analysis > is correct? Can you help me on making this code thread safer? > > Thank you very much. > > public class MyClass > { > private Person tim = new Person("Tim"); > private Person tom = new Person("Tom"); > > public void doThis > { > this.checkSomething(); > > if(tim != null) //thread "A" is running "doThis()" and > tim is not null at this moment > { > ... //other code > tim.doA(); //tim is NULL all of sudden, and > NPE is thrown. I think it is due to another thread running > checkSomething() > tim.doB(); > tim.doC(); > > } > > } > > > public void checkSomething() //thread "B" is running checkSomething() > { > if(tim.isAbsent()) > { > tim = null; > } > > if(tom.isAbsent()) > { > tom = null; > } > > } > > } The simplest and least error prone approach is to declare both methods as synchronized, e.g. synchronized public void doThis() synchronized public void checkSomething() I wouldn't consider any other approach else unless there are known, specific performance requirements that this does not meet. And making a class "thread safer" is not a goal you should aim for. If a class is not completely thread safe and it's used in a threaded environment, threading errors *will* eventually occur. You probably won't see them, but your users will. -- Jim Janney
From: www on 19 Jul 2010 13:28 Jim Janney wrote: > > The simplest and least error prone approach is to declare both methods > as synchronized, e.g. > > synchronized public void doThis() > > synchronized public void checkSomething() > This is equal to running my program in single thread. Am I correct? More specifically, how can I make sure that checkSomething() is finished at the top of doThis()?
From: Lew on 19 Jul 2010 13:39
Peter Duniho wrote: > The simplest fix is to just add "synchronized" to the method > declarations. This will cause each method to acquire the instance's > monitor during their execution, ensuring that only one thread can be > executing either method at a time. > > Another alternative would be to use the "synchronized" statement to the > code inside each method. Assuming the "isAbsent()" method isn't > affected by the "other code" or the "doA()", "doB()", or "doC()" > methods, that could look like this: > > public class MyClass > { > private Person tim = new Person("Tim"); > private Person tom = new Person("Tom"); > private final Object lock = new Object(); > > public void doThis > { > this.checkSomething(); > > synchronized (lock) > {... > } > ... > } > > } > ... > Note the use of a dedicated object reference for synchronization. In > general, it is poor practice to use the "this" reference for > synchronization, though that's what declaring a method as "synchronized" > does. It's not the end of the world to use "this", but doing so "leaks" > some of your implementation, and introduces the possibility of some > other code not related to your own taking the same monitor, increasing > contention as well as the complexity of the locking scenarios. > > Those are the simplest techniques available in Java, and for the example > you've given are probably the most appropriate anyway. As you learn > more about concurrent programming, you'll probably want to look at the > other synchronization features in Java, found mainly in the > java.util.concurrent package. > To move further after assimilating Pete's excellent advice, study / Java Concurrency in Practice/ by Brian Goetz, et al., <http://jcip.net/> and /Concurrent Programming in Java/ by Doug Lea. <http://gee.cs.oswego.edu/dl/cpj/index.html> There are also several excellent articles on concurrent programming in the Java section of IBM Developerworks. <http://www.ibm.com/developerworks/java/> And of course, you can never go wrong with Joshua Bloch's /Effective Java/ <http://java.sun.com/docs/books/effective/> For this topic, study chapter 10, items 66 through 73, inclusive. -- Lew |