Win a copy of Murach's Python Programming this week in the Jython/Python forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Synchronization on a non-final field.  RSS feed

 
Maris Orbidans
Ranch Hand
Posts: 149
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi

Intellij shows a warning every time I synchronize on a non-final class field. Here is an example:



This inspection reports instances of synchronized statements where the lock expression is a non-final field. Such statements are unlikely to have useful semantics, as different threads may be locking on different objects even when operating on the same object.


I don't fully understand what does it mean. I know that object o will be set only once and before method x() is invoked. Is there still a problem with such code ?
 
Stan James
(instanceof Sidekick)
Ranch Hand
Posts: 8791
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
For a number of threads to synchronize a block of code correctly they need to synchronize on the same object instance. That sample code would allow someone to change "o" to a new object instance between the time Thread 1 synchronizes and the time Thread 2 synchronizes. Then both threads could enter the synchronized block at the "same time". We could probably make up a scenario where this is correct behavior, but it seems a lot more likely that it is not what we would like.

Did that makes sense?
 
Maris Orbidans
Ranch Hand
Posts: 149
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There's a problem with that though - it's still possible for another thread to think that o == null after setO() has been called. That's the sort of weird thing that happens with threads if they're not properly synchronized. And in this case the synchronization only occurs after o has been accessed, which is to late to ensure that o is accessed correctly. You could fix this by making o transient, though that's bit unusual for something that isn't supposed to change. Or you could declare both setO() and x() to be synchronized - that is, synchronized on the current instance of the X class, not the o variable. In that way, you would synchronize before accessing o, and ensure that you got the correct o. For that matter, if you synchronize on the X instance, do you still need to sync on o at all? You may not need to anymore.

All in all, making o final is probably the simplest for you to do, if that's possible. We can't guess what these methods are really doing, so it's hard to offer good suggestions about what sort of redesign would make the most sense here.
 
Romeo Son
Ranch Hand
Posts: 92
Android Eclipse IDE Suse
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Maris,

Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.


Maybe you know this, but you don't have to think only to yourself. Since your method setO(Object o) is public, what does prevents another developer that uses your class to call it and change the object you are synchronizing on?

If you would have declared your instance field final, the compiler would have been forced you to initialize it in the constructor, that would have made your IDE happy I guess.
 
Mr. C Lamont Gilbert
Ranch Hand
Posts: 1170
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Maris Orbidans:
Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.


If this is the case then try setting its value in the constructor and/or using a factory method to set it.
 
Chris Hurst
Ranch Hand
Posts: 443
3
C++ Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jim presumably you mean 'transient' in its memory sense of the word rather than as Java defines it, i.e. aren't you describing volatile, or am I amount to learn some new property of transient ;-)

PS

Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.


Obviously currently you'ed have the implied condition that you object was created in and your setter called within the same thread.
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
[Chris]: Jim presumably you mean 'transient' in its memory sense of the word rather than as Java defines it, i.e. aren't you describing volatile, or am I amount to learn some new property of transient ;-)

Yes, I have an annoying tendency to say transient when I mean volatile, and vice versa.
 
Derek White
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Slight dig here, but I found this thread by Googling this exact message, so I'm going to assume others will as well.

I have ran into this same message, and I've replaced the code as such:


I was wondering A, is this okay in general, and B, would this be bad because I am getting the object twice instead of acquiring a lock then using the locked object, IE:


JSYK, I am also setting o via a synchronized mutator.

Both of these make the warnings go away, and both of them seem to be doing what I want them to, but I am wondering which is proper and why, and whether or not there will be consequences and why.


Thanks!
 
sarvesh meens
Ranch Hand
Posts: 43
Firefox Browser Java Netbeans IDE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

In the first case,the object used to lock is private & there is no way for other any other thread to change it (assuming, there is no public setter methods provided). Hence, there are no warnings from the IDE.

In the second case, you are locking on local variable. There is no way for any other thread to access the local variable of another thread.Hence, there are no warnings from IDE.

The best/suggested way to lock in the given scenario is this:


Hope, this helps.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!