bbottema/simple-java-mail

ThreadPoolExecutor terminated when sending an async mail

amanteaux opened this issue · 8 comments

amanteaux commented

Hello,

In production I had this error with simple-java-mail 4.4.5:

java.util.concurrent.RejectedExecutionException: Task sendMail process rejected from java.util.concurrent.ThreadPoolExecutor@79627f99[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 3]
        at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:2047)
        at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:823)
        at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1369)
        at org.simplejavamail.mailer.internal.mailsender.MailSender.send(MailSender.java:196)
        at org.simplejavamail.mailer.Mailer.sendMail(Mailer.java:364)

The call was: mailer.sendMail(email, true);

By looking at these classes:

There is indeed a bug:

  • in the synchronized checkShutDownRunningProcesses method, the pool is shutdown if there is no more mail to send,
  • in the synchronized send method, the pool is started if it is null or shutdown.
    But the thing is, calling the shutdown method on a ThreadPoolExecutor is not a synchronized operation, it just Initiates an orderly shutdown.

My main question is actually: why shutting down the pool if there is no mail to send?
In the comment before the shutdown is is written shutdown the threadpool, or else the Mailer will keep any JVM alive forever. But actually by default, the thread factory in the ThreadPoolExecutor build non daemon threads, so it will not keep the JVM alive.

bbottema commented

in the synchronized send method, the pool is started if it is null or shutdown.
But the thing is, calling the shutdown method on a ThreadPoolExecutor is not a synchronized operation, it just Initiates an orderly shutdown.

Hmm, it's been a while since I took a dive into Java's concurrent libraries. Won't executor.isTerminated()` return true if it is shut or shutting down? If not, then that's the problem I think; the connection pool should be re-initialized once shutdown has been called.

Actually, making it null after calling shutdown would solve it.

My main question is actually: why shutting down the pool if there is no mail to send?
In the comment before the shutdown is is written shutdown the threadpool, or else the Mailer will keep any JVM alive forever. But actually by default, the thread factory in the ThreadPoolExecutor build non daemon threads, so it will not keep the JVM alive.

I found that when sending mails from static void main (and perhaps junit test), the java process wouldn't return after sending the email and simply hang. Closing the connection pool solved that issue at the time.

amanteaux commented

Actually I said that non-deamon threads would not prevent the JVM to stop, but that is the other way around: deamon threads would not prevent the JVM to stop.
You can verify that this way the JVM is really stopped:

public static void main(String[] args) {
	ExecutorService executor = Executors.newFixedThreadPool(5, r -> {
		Thread thread = new Thread(r);
		thread.setDaemon(true);
		return thread;
	});
	executor.execute(() -> System.out.println("Async !"));
}

My point is: yes we can solve easily the issue by nulling the instance of the thread pool in checkShutDownRunningProcesses, but maybe we can "use" this issue to simplify this part. And also if the thread pool is already created, it requires less work to send an async mail (thread pool creation + thread creation).
If that is ok with you, I can do the change with a deamon-threaded pool and make the pull request. Else sure we can just make the thread pool instance null. Your choice :)

amanteaux commented

And about your question:

Won't executor.isTerminated() return true if it is shut or shutting down?

It seems that if the thread pool is shutting down, then false is returned:

public static void main(String[] args) {
	ExecutorService executor = Executors.newFixedThreadPool(5, r -> {
		Thread thread = new Thread(r);
		thread.setDaemon(true);
		return thread;
	});
	executor.execute(() -> System.out.println("Async !"));
	executor.shutdown();
	System.out.println(executor.isTerminated());
}
bbottema commented

@amanteaux, thanks for testing and by all means give it a shot!

amanteaux commented

Ok great, thank you! I will try to do that tomorrow!

bbottema commented

Just be sure to branche off from the master branch. Develop is currently waaay ahead and won't release soon. Let's aim for 5.1.6 (I just released a bugfix in 5.1.5 a minute ago)

amanteaux commented

Ok, noted, thank you!

bbottema commented

Released in 5.1.6. Solution for the 5.x.x releases is simply to check if the connection pool was shutdown instead of terminated. Also when a mail thread fails, the exception is now logged and not thrown furter up.