Hi I'm writing a domain object that will be used in a multi-threaded environment. It's properties are bound. Is it a common idiom to just make the getters and setters synchronized? Is this safe/a good idea? Would it be better to have a lock per property? Here's some code :
Hi Don, Consider a scenario where one client is setting field LoadTime and another is trying to get Name oncurrently. If you make all getters and setters synchronized, one of the above client wil wait for the other to finish which is not correct as both of them are dealing with different properties. Now, having said the above, since your code in setters does not do much, so it would not really matter in the bigger picture but for academic reasons it doesnt really sound good.
Now, just analysing your code,
- You are setting the new property and notifying the listeners for the change in property. Is it fine that a client calling a getter on a property gets the new value before the listeners are notified? OR Can the listeners be notified just before assigning the new value to your field? (The answer to the above questions will be true, if the client retrieving the value of a field is dependent on some action taken by the listener.) If the answers to the above questions are yes, then probably you dont need the synchronization of the getters and setters. You can only do with a little modification of the code. However, if the answer is no, then you need synchronization and probably having different locks for fields will make it ideal
If the object of the class is going to be shared by many threads the you need to syncronize the getters and setters to prevent data corruption.In case all the threads would have seperate copy of the object then there is no need to syncronize the methods.
You mean data consistency, not data corruption. I don't see any risk of data corruption without synchronization. However, I don't think the synchronization is giving him data consistency. He gets that from the observer pattern he implemented.
However, I do not like synchronized methods. Try instead synchronized blocks. Furthermore, I do not like firing events from within synchronized blocks. It poses certain restrictions on the receiver of the event. For instance, if the receiver happens to spawn a seperate thread, and that thread tries to read the new value, it will not be able to because the lock is held by the thread that triggered the event. Its a headache to manage that.
Joined: Nov 29, 2005
Originally posted by Mr. C Lamont Gilbert: You mean data consistency, not data corruption.
My bad ; I meant data consistency.
But still I stick to what I mentioned in my previous post.If you are planning to make object of this and if the same object is visible to multiple threads then there are changes that the data might get inconsistent , so we have to synchronize the methods so that read and write to a variable should not happen concurrently.
Yes , I do agree that we should keep the minimum number of statements in synchronized context.So syncronized blocks are more preferred approach than synchronized methods.
One thing to beware of here is that we don't know how long it takes to call those getters. The code looks short, but it calls firePropertyChange(). This in turn calls every listener for that property, sequentially. So if there are many listeners or if one of the listeners has long-running code, it will slow down responses for all other threads as well.
This would actually be true even if the methods were not synchronized, because there's also synchronization within PropertyChangeSupport, and all properties use that same shared instance of PCS, so the synchronization means none of the listeners will ever be called concurrently.
This doesn't mean that this design won't work, but it's a potential risk to beware of. Be careful not to put too much code in the listeners. (Much like awt event listeners, if you've used those). Or if you need to put a lot of code in the listeners, you may need to move to having a different PCS instance for each property. Or you could use java.util.Observable instead. Or you could even develop a more concurrent solution with a wait-notify protocol or Locks and Conditions from java.util.concurrent.locks. That's going in a rather different direction, probably not what you want, but I'm just saying there are other options if you need more concurrency.
[CLG]: For instance, if the receiver happens to spawn a separate thread, and that thread tries to read the new value, it will not be able to because the lock is held by the thread that triggered the event.
Well, the new thread will have to wait to acquire the lock. Which it does automatically anyway, right? I don't see any added complexity. Alternately, the new thread can be passed a reference to the PropertyChangedEvent which has a copy of the new value, and then there's no need to wait at all. Of course the property may have changed again after the event, but that's an inherent risk of spawning a new thread; nothing special about this design. People spawning new threads need to beware of the risks. If they need all property changes to be processed sequentially, don't spawn new threads from the listeners. [ April 27, 2007: Message edited by: Jim Yingst ]