2
\$\begingroup\$

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<T> {

    public MessageQueueHolder(int initialSize) {
        messageSet = Collections.newSetFromMap(new ConcurrentHashMap<T, Boolean>());
    }
    public MessageQueueHolder() {
        messageSet = Collections.newSetFromMap(new ConcurrentHashMap<T, Boolean>());
    }
    private final Set<T> 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<T> takeMessage() {
        final Iterator<T> 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 :)

\$\endgroup\$
  • \$\begingroup\$ I have decided to just use a ConcurrentLinkedQueue and handle duplicates elsewhere. \$\endgroup\$ – Dexter Jun 14 '15 at 1:02
2
\$\begingroup\$

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:

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

Some other notes....

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

Here's a simple synchronized version:

public class MessageClassHolder<T> {

    private final LinkedList<T> messages;

    public MessageClassHolder() {
        messages = new LinkedList<>();
    }

    /**
     * @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<T> takeMessage() {
        synchronized(messages) {
            return messages.isEmpty() ? Optional.empty() : Optional.of(messages.removeFirst());
        }
    }

    /**
     * Clear the message set
     */
    public void clear() {
        synchronized(messages) {
            messages.clear();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I get most of your arguments, but contains on a LinkedList rubs me the wrong way. Perhaps use a LinkedHashSet instead (?). \$\endgroup\$ – Veedrac Jun 13 '15 at 23:50
  • \$\begingroup\$ Let me add another alternative too.... \$\endgroup\$ – rolfl Jun 13 '15 at 23:56
  • \$\begingroup\$ Apon further reflection using a LinkedHashMap will not solve the problem I thought it would. Again, Iw ould recommend just using a LinkedTransferQueue and not worry about the duplicates. Duplicates will happen regardless of what queue you use, so it is a weak requirement anyway. \$\endgroup\$ – rolfl Jun 14 '15 at 0:05
  • \$\begingroup\$ Yes, I think using a LinkedTransferQueue is a decent approach. But for me duplicate messages is a big deal. Should I consider using a sparse array to keep track of duplicates ? \$\endgroup\$ – Dexter Jun 14 '15 at 0:07
  • 2
    \$\begingroup\$ Even with your code in your question you will get duplicates... you know that, right? someone adds message X, you take message X. Someone adds message X, you take message X, etc. \$\endgroup\$ – rolfl Jun 14 '15 at 0:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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