Message queue with multiple producer, single consumer and no duplicates I had to make a message queue in an android app which my thread takes messages from and processes them. There can be multiple producers and duplicate messages should be avoided. Rate of insertion is erratic but not fast. Rate of consumption is slow. I came up with the following : 

public class MessageQueueHolder&lt;T&gt; {

    public MessageQueueHolder(int initialSize) {
        messageSet = Collections.newSetFromMap(new ConcurrentHashMap&lt;T, Boolean&gt;());
    }
    public MessageQueueHolder() {
        messageSet = Collections.newSetFromMap(new ConcurrentHashMap&lt;T, Boolean&gt;());
    }
    private final Set&lt;T&gt; messageSet;

    /**
     * @param message to be inserted
     * @return if message already present returns false
     */
    public boolean insertMessage(T message) {
        return messageSet.add(message);
    }

    /**
     * @return returns the optional message and removes it
     */
    public Optional&lt;T&gt; takeMessage() {
        final Iterator&lt;T&gt; iterator = messageSet.iterator();
        if(!iterator.hasNext())
            return Optional.absent();
        final T message = iterator.next();
        iterator.remove();
        return Optional.of(message); //message can't be null
    }

    /**
     * Clear the message set
     */
    public void clear() {
        messageSet.clear();
    }
}


It seems to work just fine but I would appreciate some feedback :)
You code looks like it works just fine, but only because you have a single consumer thread. If you have multiple consumers, then you have a race-condition in your takeMessage method (a different thread may empty the queue after you call hasNext() but before you call next() causing an iteration exception....).

The whole iterator method also feels... clunky, and is a result of the basic Set usage.

I can't help but think that, in your situation, the use of the ConcurrentMap backing the set, is overkill. I know that HashSets have a backing Map as well, but that's different.

So, having thought some more about it as well, I also have to question the need to de-duplicate the producer messages as well in the Set... are you sure you need to?

Also, since you have the system you do, are you sure the queue will ever be big enough to worry about performance of the Set?

I can't help but feel you should go one of two ways:


Just use a LinkedTransferQueue and deal with duplicates elsewhere (use the add and poll methods respectively).
Just use a List with a simple contains() and basic synchronization.


Some other notes....


Why do you have a constructor that takes an initial size, but then does nothing?


Here's a simple synchronized version:

public class MessageClassHolder&lt;T&gt; {

    private final LinkedList&lt;T&gt; messages;

    public MessageClassHolder() {
        messages = new LinkedList&lt;&gt;();
    }

    /**
     * @param message to be inserted
     * @return if message already present returns false
     */
    public boolean insertMessage(T message) {
        synchronized(messages) {
            if (messages.contains(message)) {
                return false;
            }
            return messages.add(message);
        }
    }

    /**
     * @return returns the optional message and removes it
     */
    public Optional&lt;T&gt; takeMessage() {
        synchronized(messages) {
            return messages.isEmpty() ? Optional.empty() : Optional.of(messages.removeFirst());
        }
    }

    /**
     * Clear the message set
     */
    public void clear() {
        synchronized(messages) {
            messages.clear();
        }
    }
}