inspection message on synchronized (object_name)

Hi,
Just wondering what the intent of this inspection message is in 5.0:
"Synchronization on a non-final field is unlikely to have useful semantics".
I seem to get it every time I used synchronized (object_name) and don't get the point. Not clear on what I'd ever synchronize on a final field. In this case, eg. I'm locking down a collection so it can't be modified by other threads while I'm iterating - pretty straight-forward - why would this be an issue? Or am I just misreading this?
Thanks,
Scott

15 comments
Comment actions Permalink

Scott Johnson wrote:

Hi, Just wondering what the intent of this inspection message is in 5.0:
"Synchronization on a non-final field is unlikely to have useful
semantics". I seem to get it every time I used synchronized (object_name) and
don't get the point. Not clear on what I'd ever synchronize on a final
field. In this case, eg. I'm locking down a collection so it can't be
modified by other threads while I'm iterating - pretty straight-forward - why
would this be an issue? Or am I just misreading this? Thanks, Scott


It's usually a bad idea to synchronize on anything but your own internal lock
objects.

When you synchronize on collections like HashSet, you're not preventing
modifications to it, unless all of your code synchronizes on that set when
accessing and modifying it, and it never leaves the scope of that code.

When you synchronize on "this" you're allowing other classes to modify your
synchronization semantics by synchronizing on you.

If you want to be totally safe you will only synchronize on private final Object
fields.

0
Comment actions Permalink

ah, OK - I'll just agree to disagree and turn this one off. Yes, of course, if you lock a collection you have to make sure that all unacceptable modification (ie. add/remove) is also synchronized - I have collections designed for that. This is a large server which has many housekeeping threads and execution threads. I generally don't prefer to introduce an additional object for every object I made need to lock down. I understand the technique but I don't really buy that it adds any safety if you've properly written your code - and, in my case, some modification is acceptable. I use the technique this inspection is recommending in some cases but don't see that it's appropriate in all synchronization scenarios so it seems a bit intrusive to suggest that any use of synchronized on a non-final object is incorrect and won't provide any semantic benefit. Anyway, thanks for the response - as I say, I'll just turn this inspection off.
Scott

0
Comment actions Permalink

Scott Johnson wrote:

ah, OK - I'll just agree to disagree and turn this one off. Yes, of course,
if you lock a collection you have to make sure that all unacceptable
modification (ie. add/remove) is also synchronized - I have collections
designed for that. This is a large server which has many housekeeping
threads and execution threads. I generally don't prefer to introduce an
additional object for every object I made need to lock down. I understand
the technique but I don't really buy that it adds any safety if you've
properly written your code - and, in my case, some modification is
acceptable. I use the technique this inspection is recommending in some
cases but don't see that it's appropriate in all synchronization scenarios so
it seems a bit intrusive to suggest that any use of synchronized on a
non-final object is incorrect and won't provide any semantic benefit.
Anyway, thanks for the response - as I say, I'll just turn this inspection
off. Scott


I think your code is probably hard to read and to maintain for most developers.
It's not inherently unsafe but it's prone to error. This is why the inspection
exists. It also exists to prevent people from thinking they're safe by
synchronizing on a field while assigning it to a new value, for example.

Using synchronized the way you are, you might as well use
Collections.synchronizedSet and its friends. Then you can get rid of most of
your synchronized blocks altogether, although not when iterating.

For most concurrent collections you should use java.util.concurrent.* classes,
or the java 1.4 backport if you're stuck with 1.4. (CopyOnWriteArrayList sounds
best for your case, based on the little I heard.)

0
Comment actions Permalink

I understand the technique but I don't really buy that it adds any safety if you've properly written your code

That statement is true for all five hundred or so inspections in the product (although I would extend your caveat to "if you've properly written your code, and your team-mates have properly written their code (including that guy we fired six months ago for sniffing glue in the server room), and any frameworks you depend on don't do anything stupid"). It is quite true that following a bunch of inspections to the letter results in code that is "cleaner than it needs to be", and some of the programming to follow the coding standards involved may have a performance impact.

OTOH, the sort of encapsulation this inspection implies makes it a lot less likely that one of your co-workers will inadvertantly introduce a deadlock on one of your locks in some portion of the code you've never even heard of, waking you up at three in the morning to debug a thread dump the size of a phone book. Been there, done that, bought the caffeine tablets. I follow this one and the related "synchronized method" inspection religiously for just that reason, and have never seen enough performance impact to worry about.

Threading in Java is hard. Anything you can do to localize the amount of code you need to understand to make a thread or resource work correctly is probably worth the effort. Globally visible lock objects can be many times more dangerous than globally visible variables, and those have gone out of fashion for many good reasons (although global variables are certainly safe if you code correctly). That's what this inspection is all about.

--Dave Griffith

0
Comment actions Permalink

OK, well, I get all your input - but I'm still not convinced. Perhaps the collections example wasn't the best. Take, for example, this scenario. I have a collection of objects, each of which represents a user in a conferencing application. Now, I can have thousands of these at any given point in time - some are connected via a telephone, some are connected via a web view, some have both. Any of these can send messages to alter their state - additionally, some users can alter the state of other users (eg. the moderator of a conference can mute a participant). A pool of executor threads processes the messages received from all these users - there are also housekeeping threads that perform a variety of processes. Now, lets say a message is received which needs to alter the state of a user. The accessors are synchronized to prevent concurrent modification. Are you suggesting that a better approach is to introduce another object for locking - for each user? Of course, one for all users is completely wrong - there's no reason mutliple user objects can't be updated at the same time - you just don't want the same one being updated by multiple threads at the same time. Additionally, a housekeeping thread may need to lock a user down at some point to later it's state - which it can do by locking the user object. So, given several thousand users, you're suggesting that each have it's own additional object for locking? If so, I still don't see the benefit - or am I missing your point? If I were to use a single object, then only one user object could be modified at any given time which would kill my performance and pretty much remove the benefits of multiple message executor threads. Again, I understand the technique and I'm quite familiar with Doug Lea's work - but I still don't see how adding several thousand extra objects for locking is helpful. Yes, I am stuck with 1.4 for a while - it's a commercial product so I'll be maintaining it for quite some time. If I'm missing something, I'm always glad to get input. Also, with all due respect, if you haven't seen someone's code you really aren't in a position to state that it's not readable. Well written, self-documenting code can be very easy to read and understand, even if you don't agree with a particular coding technique used by the author.
Scott

0
Comment actions Permalink


The problem here doesn't manifest until some other developer gets a handle to one of your User objects, and starts doing his own multi-threaded coding around it. In that case, he could forget to lock on the User object, and then modify it. That's bad. Or he could lock on that User object in his code and then call some of yours, which spawns a thread and attempts to lock on the same object. That's worse. Or he can call notify() on the User object that you happen to be wait()ing on, with semantics that are allowed to vary from exactly what you expect to an atrocious trainwreck, arbitrarily and indeed whimsically. I've seen all of these in the wild. This all can be avoided if there is no way for him to see the object you're locking on, which is really easy to accomplish using this inspection. I only wish it was possible to write a quick-fix for it.

java.lang.Objects take 8 bytes in most JVMs. For a server application there's no reason to stint on private final lock objects, unless you're looking to lock tens of millions of objects. At that scale, the sort of techniques you're describing don't work very well either.

If you can gaurantee that the objects you lock on never escape your scope, then you're probably safe from anything but your own mistakes. Most programmers aren't in that boat. They need to be safe from their coworkers mistakes as well. Encapsulate your locks, for the same reason you encapsulate your instance variables and initialization logic.

--Dave Griffith

0
Comment actions Permalink

Now I'm intrigued - I'm really not being argumentative, but I don't get much of what you wrote. Sure, I understand the problem you present, but I'm unclear on an implementation of a private lock that would solve them - and I really am very interested in your solution. You say "he could forget to lock on the user object then modify it" - not if the modification methods are synchronized - I don't ever want someone to have to explicitly lock down an object just to modify an attribute - that should be transparent - no way everyone will always remember that. Then you say "or he could lock on that User object ... which spawns a thread and attempt to lock on the same object" - OK, so new thread is blocked - but how would that be different if there was a private variable doing the locking - the new thread would be just as blocked. I guess I'm just not clear on implementation. If several attributes need to be changed, each with their own setX() method, you need to explicitly synchronize something so that all the sets are done safely - you can't write a method for every possible combination of attributes that need to be modified - and I can't see writing methods that take name/value pairs of attributes to be modified. So I'm still not getting where the lock is? A private variable inside my user object? That doesn't work - every attribute change is atomic with the lock being released between each one - doesn't handle several attribute changes in a logical unit of work. If the thread dealing with the user object doesn't explicitly synchronize on something, then it's got to be transparent - inside the object - but that only works for very atomic modifications. So, from a coding standpoint, how would you implement this - where is the lock variable and who actually synchs on it?

If I've got:

>
   

   public void setX(int x) {
      this.x = x;
   }

   public void setY(int y) {
      this.y = y;
   }

   <>
}

... somewhere in a server

confUser user = new confUser;

]]>



Now, some thread needs to modify x, some thread needs to modify y, some thread needs to modify x and y (maintaining the lock until both fields are set) and some housekeeping thread may need to grab this object and guarantee no attributes are changed while it has the object, even if it's not going to change an attribute - where is the locking object and how is it grabbed?

I'm not concerned about heap usage - I'm just not getting how the approach this inspection suggests provides a better solution this kind of problem. I'm always VERY concerned with simplifying locking - forget other developers, I've burned myself plenty - anyone who's gone through the learning curve of concurrent access, I'm sure, can relate to that!

Oh, BTW, you using Idea 5.0 yet? I was just looking through the list of inspections (settings/errors) - notice there's a filter box but you can only enter 1 character in it - not sure if that's a bug or not.

Thanks again for the input.
Scott

0
Comment actions Permalink

To me the obvious solution is to encapsulate all of the mutations which
currently require the caller to lock. For simplicity I will use synchronized
methods even though for the most safety you should use private locks.

class ConfUser {
private int x;
private int y;

public synchronized void set(int x, int y) {
this.x = x;
this.y = y;
}
}

This way, the only locking is done internally by the user object itself, which
is usually easier to maintain and easier to find bugs in. Why doesn't this work
for you?

Scott Johnson wrote:

Now I'm intrigued - I'm really not being argumentative, but I don't get much
of what you wrote. Sure, I understand the problem you present, but I'm
unclear on an implementation of a private lock that would solve them - and I
really am very interested in your solution. You say "he could forget to lock
on the user object then modify it" - not if the modification methods are
synchronized - I don't ever want someone to have to explicitly lock down an
object just to modify an attribute - that should be transparent - no way
everyone will always remember that. Then you say "or he could lock on that
User object ... which spawns a thread and attempt to lock on the same object"
- OK, so new thread is blocked - but how would that be different if there was
a private variable doing the locking - the new thread would be just as
blocked. I guess I'm just not clear on implementation. If several
attributes need to be changed, each with their own setX() method, you need to
explicitly synchronize something so that all the sets are done safely - you
can't write a method for every possible combination of attributes that need
to be modified - and I can't see writing methods that take name/value pairs
of attributes to be modified. So I'm still not getting where the lock is? A
private variable inside my user object? That doesn't work - every attribute
change is atomic with the lock being released between each one - doesn't
handle several attribute changes in a logical unit of work. If the thread
dealing with the user object doesn't explicitly synchronize on something,
then it's got to be transparent - inside the object - but that only works for
very atomic modifications. So, from a coding standpoint, how would you
implement this - where is the lock variable and who actually synchs on it?

If I've got:

 
> <>
> 
> 
> public void setX(int x) { this.x = x; }
> 
> public void setY(int y) { this.y = y; }
> 
> <> }
> 
> ... somewhere in a server
> 
> confUser user = new confUser;
> 
> ]]>


Now, some thread needs to modify x, some thread needs to modify y, some
thread needs to modify x and y (maintaining the lock until both fields are
set) and some housekeeping thread may need to grab this object and guarantee
no attributes are changed while it has the object, even if it's not going to
change an attribute - where is the locking object and how is it grabbed?

I'm not concerned about heap usage - I'm just not getting how the approach
this inspection suggests provides a better solution this kind of problem.
I'm always VERY concerned with simplifying locking - forget other developers,
I've burned myself plenty - anyone who's gone through the learning curve of
concurrent access, I'm sure, can relate to that!

Oh, BTW, you using Idea 5.0 yet? I was just looking through the list of
inspections (settings/errors) - notice there's a filter box but you can only
enter 1 character in it - not sure if that's a bug or not.

Thanks again for the input. Scott

0
Comment actions Permalink

Scott Johnson wrote:

Now, some thread needs to modify x, some thread needs to modify y,
some thread needs to modify x and y (maintaining the lock until both
fields are set) and some housekeeping thread may need to grab this
object and guarantee no attributes are changed while it has the
object, even if it's not going to change an attribute - where is the
locking object and how is it grabbed?


Threads which need to modify x and y as a single atomic operation should
call setXandY(int x, int y) which does the appropriate locking which
covers setting both fields.

If I understand correctly, the reason that Dave and Keith are
recommending a private lock object is that if your confUser object is
your lock object, then clients of confUser can cause it to become locked
in situations for which you didn't/cannot plan/design. If clients of
confUser cannot access your lock object and you ensure that all public
access to confUser performs the correct locking internally (see example
below), your life is simpler and you only have to worry about deadlocks
caused by your own code.


Ciao,
Gordon

--
Gordon Tyler (Software Developer)
Quest Software <http://www.quest.com/>
260 King Street East, Toronto, Ontario M5A 4L5, Canada
Voice: (416) 933-5046 | Fax: (416) 933-5001

0
Comment actions Permalink


Ah, now I see the issue. There are several solutions, none of them perfect.

1) Package up all of the access patterns that you actually wish to use as separate methods. Preferably those methods should each correspond to some real business logic which needs to be performed atomically. This is the optimal solution, but in the case of bean-like objects may result in many methods. That's just another reason to avoid bean-like objects with a lot of setters, but some patterns (DAO, in particular, and some MVC implementations), don't let you do that.

2) Create generic getState() and setState() methods, which allow me copy all of my object's state into some local or thread-local storage, and then reset it in bulk. Unquestionably ugly, but probably not as bad as you think, and can be reused for remoting or transactional uses. It's also great when you need to get a snapshot of the object's state for some expensive processing, and don't want to lock for the duration. Try 1) first, and punt to this for edge cases.

3) If I do need to have my objects expose this sort of session-locking, I often prefer to do so explicitly. Have a lock() and unlock() method, which access a java.util.concurrent.lock object stored internally to my object. The user is then responsible for calling lock() on the object before modifying it, and unlock when they are done (ideally in a finally block). This looks complicated, but all that's really happening is that you're exposing the complexity that simply synchronizing on the object was hiding, but would probably someday arise to bite you. It makes possible a bunch of stuff that you can't do easily with the stock monitor semantics, like balking, timeouts, logging, performance tracing, and deadlock detection. Less safe than 1), but still more safe than synchronizing on the object, as you have more visibility on what could be synchronizing on you, and are protected from rogue wait() and notify() calls. Definitely overkill, though, if you've got a million beans to protect.

Between those, I'm usually able to find a way to protect my object's state, and not require the object's clients to guess at how they should safely interact with the object. Your pattern, where the user needs to know that a synchronized block will safely allow them a stateful session, but that they can safely have a single access with just a method call, seems just a bit error-prone for my tastes.

--Dave Griffith

0
Comment actions Permalink

Right, but the example I give is, of course, extremely simplified. In the real world, my user object has some 75 or so attributes, any number of which can be modified by a single message - and that's just one domain object - there are many others, as well as collections of them. You're suggesting that I write an update type method for every possible combination of attributes - this is not realistic - and imagine the nightmare of adding an attribute - instead of just adding one set method, you have to add thousands! And this is still a simplification - any number of messages may require state changes to multiple objects, all of which must be done as a single, atomic unit of work. No, the lock(s) must be grabbed and multiple attributes maintained in a single logical unit of work - atomic attribute setting with lock/unlock between each is unsafe. I suppose I could write some single method which takes a collection of name/value pairs to do the attribute modification but that seems like overkill to me and it doesn't handle multiple objects which must be maintained together. Additionally, in the single object case, assuming you do have a method for all the changes you need, I don't see how explicitly synchronizing on an internal private variable is more appealling than declaring your set method as synchronized. Much cleaner, in my mind to use a synchronized set method. In addition, this does not solve the problem of a housekeeping thread which needs to grab the object and guarantee that no other threads modify it while it has the object. In my case, for example, I have to have a hot-swappable server running and being kept in synch at all times - a watch-dog process sits over the servers - if it detects failure (not my code of course - a hardware failure!), then the other box grabs the public IP and the alternate server is ready to go - it's been kept in synch. I occasionally need to snapshot the state of an object or do other interogation with a guarantee that the state of the object not change while I have it - even if I'm not going to modify it - this approach does not allow me to do that. In my case, this is much more akin to an app server - large pieces of a running application live here and each message (typically 5 - 7 k SOAP messages per minute) can alter the state of one or more objects with the state maintained.

Anyway, I appreciate the input - and we're obviously way off the original topic. One way isn't always the best way. Guess it's good I can turn some of these new inspections off - I'm kind of concerned about several of them, actually - some of them appear to me to be personal style - different, but not necessarily better (or worse) - just different - not overly happy about having a boat-load of yellow lines around my editing view so I'll turn some of them off. Most (at least the ones I've looked at in the settings/errors dialoge) are very good and I'm glad to have them - but some of them seem to be intrusive and not necessarily better.

Again, thanks for all the input. This was a beast of a server to write, with an extremely demanding customer - and a team of one - I'm always eager for other opinions and multiple eyes - but, as I'm sure many of you know - not always easy to get in little company!
Scott

0
Comment actions Permalink

See my response to Gordon Tyler below who suggests the same thing.
Thanks for the input,
Scott

0
Comment actions Permalink

Again, that's for the response and I do appreciate the second set of eyes, so to speak. As I responded to Gordon above, this was a beast of a server with a team of one in a very small company, so additional input is always welcome. Re. your suggestion number 3, yes, when I go to jdk 5 much will be different. I looked at using some of Doug Lea's stuff in his concurrent utils package with 4.0, and in fact I do subclass his thread pool for my executor threads, but much was already implemented and there really was no time to make any changes before delivery - when I can start to shift to 5.0 I'll use them as implemented in the util.concurrent package.
thanks again,
Scott

0
Comment actions Permalink

on, and you're right, of course, with suggestion 1 - which I use whenever possible - for example, several attributes change one a caller is muted - there's a synchronized mute() method which handles this use case. But, some messages update state in random ways, such as, for example, a web client change some of his/her personal info - no way to know what fields will be changes in advance. Your number 2 suggestion is also one I use to some extent - I avoided a set type method which takes, eg. a collection of name/value pairs, but I do have snap-shot type get methods for exactly the point you make -remoting (in my case to a hot-swappable back-up instance) which are also synchronized. But here again, I don't see the benefit of explictly synchronizing on an additional object over declaring the method synchronized. And, again, with some housekeeping threads, my inspection needs aren't quite so simple - ie. based on some values I need to see others. I probably could handle that in multiple methods but the convenience of the housekeeping thread just grabbing the lock, to my mind, anyway, keeps things simpler.

Again, thanks for taking the time to hash some of this out.
Scott

0
Comment actions Permalink

Cool, we seem to worked ourself to a state of understanding, if not complete agreement. If you come up with some way of weakening the inspection which would satisfy your needs, or a different inspection entirely that would fit, please post it here. The inspections that ship with IDEA (including the one in question) were mostly developed by me, although I am not a JetBrains employee and am not currently maintaining them. If there's some way of improving them that you can think of, it's in everyone's interest to ask for it.

Good discussion. It's always interesting to see the tension between code audits as presented in theory and what actually occurs in the wild.

--Dave Griffith

0

Please sign in to leave a comment.