Which blocks of code should be synchronized?

Question

I have three different classes:

1. Managed bean (singleton scope)
2. Managed bean (session scope)
3. Spring @Controller

I read few posts here about synchronization, but I still don't understand how it should be and how it works.

Short examples:
1) Managed bean (singleton scope).
Here all class fields should be the same for all users. All user work with one instance of this object or with his copies(???).

    public class CategoryService implements Serializable {
private CategoryDao categoryDao;
private TreeNode root; //should be the same for all users
private List<String> categories = new ArrayList<String>();//should be the same for all users
private List<CategoryEntity> mainCategories = new ArrayList<CategoryEntity>();
//should be the same for all users

public void initCategories() {
    //get categories from database
}

public List<CategoryEntity> getMainCategories() {
    return mainCategories;
}}  

2) Managed bean (session scope)
In this case, every user have his own instance of object.
When user trying to delete category he should check are another users which trying to delete the same category, so we need to use synchronized block???

public class CategoryServiceSession implements Serializable {
private CategoryDao categoryDao;
private CategoryService categoryService;

private TreeNode selectedNode;

public TreeNode getSelectedNode() {
    return selectedNode;
}
public void setSelectedNode(TreeNode selectedNode) {
    this.selectedNode = selectedNode;
}

public void deleteCategory() {
    CategoryEntity current = (CategoryEntity) selectedNode.getData();

    synchronized (this) {
        //configure tree
        selectedNode = null;
        categoryDao.delete(current);
    }
    categoryService.initCategories();
}}  

3) Spring @Controller
Here all user may have an instance (or each user have his own instance???). But when some admin try to change parameter of some user he should check is another admin trying to do the same operation??

@Controller
@RequestMapping("/rest")
public class UserResource {
   @Autowired
   private UserDao userDao;

   @RequestMapping(value = "/user/{id}", method = RequestMethod.PUT)
    public @ResponseBody UserEntity changeBannedStatus(@PathVariable Long id) {
        UserEntity user = userDao.findById(id);
        synchronized (id) {
           user.setBanned(!user.getBanned());
           userDao.update(user);
        }
    return user;
    }
}

So, how it should to be?

Sorry for my English.

Answer 1

In the code that you've posted -- nothing in particular needs to be synchronised, and the synchronised blocks you've defined won't protect you from anything. Your controller scope is singleton by default.

If your singletons change shared objects ( mostly just their fields) then you should likely flag the whole method as synchronised.

Method level variables and final parameters will likely never need synchronization ( at least in the programming model you seem to be using ) so don't worry about it.

The session object is guarded by serialisation, mostly, but you can still have data races if your user has concurrent requests -- you'll have to imagine creative ways to deal with this.

You may/will have concurrency issues in the database ( multiple users trying to delete or modify a database row concurrently ) but this should be handled by a pessimistic or optimistic locking and transaction policy in your DAO.

Luck.

Answer 2

Generally speaking, using synchronized statements in your code reduces scalability. If your ever try to use multiple server instances, your synchronized will most likely be useless. Transaction semantics (using either optimistic or pessimistic locking) should be enough to ensure that your object remains consistent. So in 2 und 3 you don't need that.

As for shared variables in CategoryService it may be possible to synchronize it, but your categories seem to be some kind of cache. If this is the case, you might try to use a cache of your persistence provider (e.g. in Hibernate a second-level cache or query cache) or of your database.

Also calling categoryService.initCategories() in deleteCategory() probably means you are reloading the whole list, which is not a good idea, especially if you have many categories.