From: Peter Duniho on 20 Jul 2010 04:25 Chintan(Neo) wrote: > Hi www, > > I read your code. i would like to ask some questions regarding the > code? have you tried the code and is it giving NullPointerException or > it is ur assumption? b'se as the class or method or objects are not > 'static', every instance of the class will have its own variables and > so it should be thread safe. 'this.checkSomething();' will make tim/ > tom null but only of that object/thread and not of other objects/ > threads. I dont find any problem in the code. I am not very > experienced at multi threaded programming but this is my primary and > logical thinking says. Please check and let me know. There's not enough context in the question to say for sure. But given the description in the original question, it seems safe to assume that both threads are using the same instance of the MyClass class. Thus, while it's true that "every instance of the class will have its own variables", that fact in no way helps avoid thread safety problems. Instances are not tied to threads. Any given instance can have code executing in any given thread. Pete
From: Chintan(Neo) on 20 Jul 2010 10:36 Hi Peter, As you said, instances are not tied to threads which is correct. however it was my assumption that instance of MyClass was created inside the thread after the thread started and i gave my view according to that. But ya, if the instance of MyClass is created before the thread started and passed to the threads, it is definately not thread safe and it seems that 'synchronized' is the option which should be used. On Jul 20, 1:25 pm, Peter Duniho <NpOeStPe...(a)NnOwSlPiAnMk.com> wrote: > > There's not enough context in the question to say for sure. But given > the description in the original question, it seems safe to assume that > both threads are using the same instance of the MyClass class. Thus, > while it's true that "every instance of the class will have its own > variables", that fact in no way helps avoid thread safety problems. > > Instances are not tied to threads. Any given instance can have code > executing in any given thread. > > Pete
From: Tom Anderson on 20 Jul 2010 13:44 On Mon, 19 Jul 2010, www wrote: > public class MyClass > { > private Person tim = new Person("Tim"); > private Person tom = new Person("Tom"); > > public void doThis > { > this.checkSomething(); > Insert this here: Person tim = this.tim; > 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; > } > > } > > } That change will prevent NullPointerExceptions emanating from doThis. The point is that by copying the instance field into a local each thread gets its own copy, which is insulated from suprising effects. You still have a risk of NullPointerExceptions from checkSomething - what if a thread enters that while either tim or tom is already null? I assume your code is actually doing null checks on the variables as well as calling isAbsent(). You should protect those in the same way as the code in doThis. However! None of this will actually make the code threadsafe, probably, because it would still be possible for A to come in and set tim to null, then for B to come in and see an old value for tim and do something with it. To avoid that, i think it's sufficient to declare tim volatile. tom -- isn't it about time we had some new label for people interested in technology who also have an interest in drinking binges, womanising and occasional bouts of ultra violence? -- D
From: Peter Duniho on 20 Jul 2010 14:03 Tom Anderson wrote: > On Mon, 19 Jul 2010, www wrote: > >> public class MyClass >> { >> private Person tim = new Person("Tim"); >> private Person tom = new Person("Tom"); >> >> public void doThis >> { >> this.checkSomething(); >> > > Insert this here: > > Person tim = this.tim; > >> if(tim != null) //thread "A" is running "doThis()" and tim is >> not null at this moment >> [...] > > That change will prevent NullPointerExceptions emanating from doThis. > The point is that by copying the instance field into a local each thread > gets its own copy, which is insulated from suprising effects. That assumes, however, that "tim == null" is not also an implicit flag indicating that the code should not be calling methods on the object previously referenced by "tim". It avoids the NPE for sure. But whether it makes the code thread-safe is an entirely different question. > You still have a risk of NullPointerExceptions from checkSomething - > what if a thread enters that while either tim or tom is already null? I > assume your code is actually doing null checks on the variables as well > as calling isAbsent(). You should protect those in the same way as the > code in doThis. Noting, of course, that issue isn't related to thread-safety. Rather, even in a single-threaded program the original code could potentially have a problem with that. > However! None of this will actually make the code threadsafe, probably, > because it would still be possible for A to come in and set tim to null, > then for B to come in and see an old value for tim and do something with > it. To avoid that, i think it's sufficient to declare tim volatile. Again, it depends on what the code is supposed to do and what's legal. Using "volatile" will ensure that the most up-to-date value for the variable "tim" is retrieved by the thread calling "doThis()". But, if it's okay to store the value to a local and then use the local, it may well be okay to get an out-of-date version of the instance field too. On the other hand, if it's not okay to get an out-of-date version of the instance field, then it very well may not be okay to use the Person object after the up-to-date version of the instance field has been set to null, in which case full synchronization is required. Of course, the third possibility is that it's not okay to get an out-of-date version, but it is okay to operate on an out-of-date version, in which case the modification you suggest works fine. Unfortunately, the OP didn't provide enough context for anyone to know what's really safe, legal, or correct with regards to his program. Any of those three possibilities are valid. Pete
From: Alan Gutierrez on 20 Jul 2010 17:57
Peter Duniho wrote: > Arne Vajh�j wrote: >> On 19-07-2010 13:08, Peter Duniho wrote: >>> 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. >> >> Someone wrote this many years ago for C#/.NET. >> >> Everybody in the C#/.NET world has repeated it since then. >> >> But no one seems to have ever seen a real world case where >> it was a problem. >> >> It is not best practice to avoid it. >> >> Best practices is about avoiding real problems. >> >> It just an urban legend type of thing. > > Yes, you've said all that before (in the C# newsgroup). You're still > wrong. > > If you like, we can just copy/paste the back-and-forth we had there to > this newsgroup. I'd be interested to see that. I'd tend to agree with Arne. I generally avoid holding the monitor on an object whose class is not part of the code base, that is, I know that there is a monitor on every object, so I don't try to hold the monitor on an instance someone else's class, because that there for them to use. The encapsulation as proscribed by the C# "best-practice" makes it impossible, I understand, but I really cotton to Arne's comment on "best-practices". So, other than the ounce of prevention, what makes Arne wrong? (Or, link to the previous discussion if you're not interested.) -- Alan Gutierrez - alan(a)blogometer.com - http://twitter.com/bigeasy |