#204 do not shutdown the ThreadPoolExecutor when mails are sent

[amanteaux]

There are 3 changes:

• The ThreadPoolExecutor is not shutdown anymore when mails are sent
• Threads created in the ThreadPoolExecutor are set to daemon (so they don't "block" the JVM shutdown) ; and also they are named "Simple Java Mail async mail sender #N" so they can be easily identified in the application threads
• If an error occurred in the async mail, the error is now logged with a logger in the MailSender class (instead of a e.printStackTrace() in the ThreadPoolExecutor): that is not directly related to the original issue, if necessary I can do a separate pull request for that

There are 2 unit tests I wanted to write (and eventually I did not):

• To reproduce the original issue:
  1. the MailSender needs to be created
  2. a first mail needs to be sent
  3. when we are sure the mail is sent, a second mail needs to be sent as fast as possible
     => but actually to make this work, I guess we would need a test SMTP server like https://github.com/davidmoten/subethasmtp ; it seems too complicated
• To check that the JVM can stop and is not blocked by daemon threads: but actually in JUnit tests, the JVM is killed event if there are non-daemon threads alive. I tried to check directly if JVM threads are all daemon using Thread.getAllStackTraces().keySet(), but JUnit seems to create many threads that are actually non-daemon... So I also gave up to do this Unit test.

However, I did make manual "real-world" tests with our Java application.

Lastly, I am wondering if the default thread pool size for ThreadPoolExecutor, 10, is not a bit too high. I think that a default pool of 2 would be enough in the majority of use cases. Is there any reason for using a default thread pool that high?

[bbottema]

Analyzing your changes right now. Can you help me understand why it is better to log the error rather than have it bubble up? I can argue the whole thing should fail when an email fails and also why it shouldn't. Depends on the use case. Maybe it should be configurable.

Actually, for v6.0.0 in develop, send returns a Future and Promise, which contains any exception that was thrown when sending async.

[bbottema]

The original default thread factory consulted the SecurityManager to determine the thread group, shouldn't your version do this as well? I must admit I'm not familiar with this function:

SecurityManager s = System.getSecurityManager();
group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();

[bbottema]

Actually, the more I look at it, the more I am starting to have doubts on daemon threads altogether. They shouldn't block the JVM from shutting down, but only when they are not actually doing anything. Now what happens is if an application is running just to send emails async, or from junit test, or from CLI (coming in 6.0.0), then the JVM kills the threads before they send anything, correct? They may even halt in the middle of an SMTP session.

By their nature I feel an emailing thread should never block the JVM. The only way then to have the JVM exit is to properly shutdown the executor.

/edit: then there's this: https://stackoverflow.com/a/29453160/441662

It solves the problem but feels like a workaround when I don't have the use case for daemon threads to begin with.

[bbottema]

Regarding my earlier question about .isTerminated(), that should have been .isShutdown(). It doesn't need to be made null.

[bbottema]

I really appreciate your effort @amanteaux, but thinking about it more I feel this is not the right path. However, I am keeping some of your changes, like the custom thread factory to fix the thread names.

[amanteaux]

I agree that the daemon thread solution is not really that good. From a JUnit test, it won't make a difference if you are using daemon threads or not: the thread pool will get shutdown/killed anyway once the test method is executed. But I can see for sure a use case where an application that executes for a short amount of time and wants to send asynchronous mails.

Wouldn't the solution with the non-daemon threads and the time out (allowCoreThreadTimeOut) be a good alternative? If the timeout is configurable:

• if by default this timeout is low, the behavior will be a lot like what exists already: threads will get killed once they have no more mails to send,
• if some application do not want to waste time creating new threads each time it needs to send mails, it can be configured to have a long timeout.

This solution should not add complexity to Simple Java Mail no?

[bbottema]

I'm willing to take another look if you are willing to take another stab at it (but branch off from "204-fix-race-condition-improve-threading" this time). Don't bother wiring the config into everything, I'll take care of that if I'm happy with the solution. I can see an advantage of a configurable timeout, although at the same time that adds complexity to Simple Java Mail as well!

[amanteaux]

About your two other questions:

• About the thread group, to be honest I do not understand why the default thread factory is duplicating what is already done at the Thread level. See Thread.init() from line 374.
• For the exception bubbling up, in the non-async context, it should of course bubble up, but in the async context, it just bubble up to the Thread, kill it at the same time and gets logged by the JVM using something like e.printStackTrace(). At least one issue I see with that behavior is that is prevent logging system like Logback to log the exception where the application wants. On my side the real issue is that exceptions do not get pushed to Elasticsearch/Kibana if they don't get logged by Logback.

And thank you for time reviewing my merge request!

[amanteaux]

Ok thank you! I will try to do something in the "204-fix-race-condition-improve-threading" branch!