MailQueue - follow up

Follow up of this question.

Things altered:

• Put logging (debug level) for creating and starting thread.

• A separate thread for starting the different threads.

• Locking object and synchronise for starting the threadIncreaser.

• Changed the while condition of the while loop of threadIncreaser so getting the size of the queue only happens when we don't have the maximum threads running.

• Changed run to running

• Removed the interface runnable.

```
public enum MailQueue {

INSTANCE;

private JavaMailSender sender;
private boolean running = false;

private final Thread threadIncreaser = new Thread(new Runnable() {
@Override
public void run() {
LOGGER.debug("ThreadIncreaser started");
int currentThreads = CURRENT_THREADS_SEND_MAIL.get();
while (currentThreads (MAX_ELEMENTS_BEFORE_NEW_THREAD * currentThreads)) {
new Thread(createSendMailsThread(currentThreads + 1)).start();
currentThreads = CURRENT_THREADS_SEND_MAIL.incrementAndGet();
LOGGER.debug(("Thread " + currentThreads + " created"));
}
threadIncreaserRunning = false;
}
});
private boolean threadIncreaserRunning = false;
private static final Object THREAD_INCREASER_LOCK_OBJECT = new Object();

private final ConcurrentLinkedQueue mailsToSend = new ConcurrentLinkedQueue();
private final ConcurrentLinkedQueue errorRun = new ConcurrentLinkedQueue();
private final Map mailsWithErrors = new ConcurrentHashMap();

private static final Logger LOGGER = LoggerFactory.getLogger(MailQueue.class);
private static final int WAIT_FAILURE_TIME = 120000;
private static final int MAX_THREADS_SEND_MAIL = 4;
private static final int MAX_ELEMENTS_BEFORE_NEW_THREAD = 25;
private static final AtomicInteger CURRENT_THREADS_SEND_MAIL = new AtomicInteger(0);

/**
* Adding a mail to the Queue. When Queue is not started, it w

In some cases this is a reiteration of the review you were given on the original question, some parts I found myself.

As Pimgd already stated in his review:


Do it the proper way. Have one class that keeps track of the tasks and
one class that does the tasks. Not this self-forking madness where you
keep a reference to the main instance by a enum variable.

He's completely right. Your design for this class is madness. What you seem to be looking for is a Facade-Structure. What you should do to accomplish that is: Create an interface, exposing the methods you need, and start from there.

public interface MailQueue {
    boolean addMail(MimeMessage message);
    boolean addMails(Collection messages);
    void resendErroredMails();
    void retrySingleErroredMail(MimeMessage message);
    boolean isRunning();
    MimeMessage createMimeMessage();
}

I left out some of the public methods and slightly changed a few others. For one: You shouldn't expose the internal workings of your MailQueue, and you also probably shouldn't allow "externals" to change it.

But that's what you do in: getMailsWithError, setSender (and also getSender), startErrorThread, removeMailFromError.

getMailsWithError should return an immutable Collection, if you want to implement it. I seriously doubt the need for it, though.

The JavaMailSender you're using is begging to be made final, since it shouldn't need to change.

Programming against Threads is hard. As Eric Lippert stated in a comment on his blog:


Many people think of threads as units of work, but they are not. Threads are workers. Most of the problems you see in multithreaded systems have analogous problems in single-threaded systems. People are just not yet in the habit of mentally separating workers from work.

What does that mean for you? Well... Use existing solutions for the Problem of "I have work I need done multithreaded", namely a ThreadPoolExecutor, because that thing is what you tried to reinvent here. The whole "Thread limit" and "more threads for more work" problematic can be solved using that thing.

In the end the functionality you have boils down to:

accept data to send
try sending it
if sending it fails
    put it into a collection that can be scheduled for resending

What you need for that is a single BlockingQueue, the JavaMailSender and a ThreadPoolExecutor. If you want to be fancy you can make the resending asynchronous.

StackExchange Code Review Q#88762, answer score: 5