Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
public interface GeneratorCancelledRemote {
    public boolean isJobCancelled(String jobId);

The server uses this interface:

import java.util.Collection;
import javax.ejb.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;
public class GeneratorCancelled implements GeneratorCancelledLocal, GeneratorCancelledRemote {
    private Collection<String>  jobs;
    public void setJobs(final Collection<String> jobs) { = Collections.synchronizedSet(new HashSet<String>());;
    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): which also explains that synchronized is not allowed in EJB.

share|improve this question
I don't know much about EJB, but the article looks horrible to me. I know quite a lot about concurrency in Java SE, and after reading the article I still don't know anything about concurrency in Java EE. – maaartinus May 19 at 0:26

2 Answers 2

up vote 1 down vote accepted
  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 = 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.
share|improve this answer
2: Collections.synchronizedSet does not create a new object (the collection is not cloned), only a view. 3&4: As I say, the EJB-gurus here balk on the use of synchronized in beans. – Martin Schröder Nov 14 '12 at 12:28
Of course Collections.synchronizedSet does not create a new object. I'm talking about new HashSet<String>(). – stoweesh Nov 14 '12 at 20:02
Actually, Collections.synchronizedSet does create a new tiny object, but I see what you mean. The thing with new HashSet<String>() is defensive copy. Here, it's needed to ensure the stored collection doesn't change sneakily. It's strictly necessary here: If you work on a provided collection to which someone has unlimited unsynchronized access, then any synchronization is futile. – maaartinus May 19 at 0:32

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); = 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.

share|improve this answer

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.