Querying a list of jobs on a server using EJBs

Question

Inspired by this question, I have a Java Enterprise application running in Glassfish 2.1 and a Java SE client that communicates with the server application.

For this I have 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.

I want to ask if this code is good.

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 around here shun on the use of synchronized in an EE context.

I found this (in German), 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.

Answer 2

Is the use of Collections.synchronizedSet the best way to do this?

No way. It allows you to do some operations atomically, e.g., addAll, but what you'd need is to replace the whole content by e.g. clear and addAll executed atomically together.

But there's a simple way to get rid of your race condition:

public void setJobs(final Collection<String> jobs) {
    Collection<String> copy = Collections.synchronizedSet(new HashSet<String>());
    copy.addAll(jobs);
    this.jobs = copy;
}

As long as you keep you collection private to the method, there can be no race.

However, it's still wrong, namely because of the second bug. If one thread writes jobs, there's no guarantee that another one sees the new value. It may see the old value for an indefinite amount of time.

To the rescue, you could declare jobs as volatile. Or use an AtomicReference or whatever.

Then it works, however, there are no operation on your set which would need it to be synchronized. The only writing happens to a local variable constrained to a single thread, the safe publication happens via volatile, and contains is reading only of an effectively immutable object and needs no synchronization.

So there's no use for Collections.synchronizedSet here. Anyway, I strongly suggest to use the synchronized keyword (or equivalents provided by EJB) as it's the least error prone way. Note that even in these few lines there were two serious bugs. And debugging it is a real pain as it's all inherently non-deterministic.