Take the 2-minute tour ×
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;
@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.

share|improve this question
add comment

1 Answer

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

    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.
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
1  
Of course Collections.synchronizedSet does not create a new object. I'm talking about new HashSet<String>(). –  stoweesh Nov 14 '12 at 20:02
add comment

Your Answer

 
discard

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.