Synchronization in EJBs

Question

Inspired by Is this synchronized correctly? I want to ask if this code is good.

I have an Java Enterprise application running in Glassfish 2.1 and a Java SE client that communicates with the server application.

For this I hava a bean that maintains a collection of jobs that is set by the server and the client then asks if his job is in the current list of jobs known to the bean.

The client uses this interface:

import javax.ejb.Remote;
@Remote
public interface GeneratorCancelledRemote {
    public boolean isJobCancelled(String jobId);
}

The server uses this interface:

import java.util.Collection;
import javax.ejb.Local;
@Local
public interface GeneratorCancelledLocal {
    public void setJobs(final Collection<String> jobs);
}

And here's the implementation:

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import javax.ejb.Stateless;
@Stateless
public class GeneratorCancelled implements GeneratorCancelledLocal, GeneratorCancelledRemote {
    private Collection<String>  jobs;
    public void setJobs(final Collection<String> jobs) {
        this.jobs = Collections.synchronizedSet(new HashSet<String>());
        this.jobs.addAll(jobs);
    }
    @Override
    public boolean isJobCancelled(final String jobId) {
        return jobs != null && jobs.contains(jobId);
    }
}

Obviously access to the collection of jobs has to be synchronized somehow. Is the use of Collections.synchronizedSet the best way to do this? The EJB gurus aorund here shun on the use of synchronized in an EE context.

EDIT: I found this (in german): http://blog.holisticon.de/2010/11/synchronisation-und-nebenlaufigkeitskontrolle-mit-ejbs-leicht-gemacht/ which also explains that synchronized is not allowed in EJB.

Answer 1

1. Your @Stateless service contains a shared state. It looks weird for me.

2. You never modify a state of jobs, you always create a new object

   this.jobs = Collections.synchronizedSet(new HashSet<String>());
   

   Could you please clarify why ?

3. From synchronization standpoint you have a race condition in setJobs method, so it should be synchronized.
4. Now, since your write operation (setJobs method) is protected by the intrinsic lock, all read operations should be also protected by the same lock to ensure data visibility. Make isJobCancelled synchronized too.