Add timeouts to SMTP transport sockets

[amanteaux]

This merge request is related to issue #85.



So actually the timeouts added for transport sockets will be used for synchronous and asynchronous emails.

These timeouts can be configured via the property simplejavamail.defaults.mailsockettimeoutinmillis. The default is 1 minute.



At first I tried to implement the solution described at https://stackoverflow.com/a/2759040/3790208, but I realized it didn't work with raw Socket objects :(. That's when I discovered that various options including timeouts can be passed to the javax-mail socket factory SocketFetcher. So since my implementation would complicate MailSender, I put it aside.

However controlling timeout at the thread level enables to configure the timeout for the whole send mail process instead of the timeout of each socket operation.

So if for whatever reason you want to control the timeout at the thread level, you can:

- tell SocketFetcher to use SocketChannel which is an interruptible Socket trough the property mail.smtp.usesocketchannels,

- replace the ExecutorService in MailSender by TimeoutTaskThreadPoolExecutor: https://gist.github.com/amanteaux/64c54a913c1ae34ad7b86db109cbc0bf

[bbottema]

Nice work!

One remark though. I deliberately had chosen for Threads rather than Runnables, because you can name them. That was very useful during debugging.

So either Runnables should add enough value to counterbalance the missing names, or they should support names somehow.

[amanteaux]

I am not sure I understood you correctly when you say you can name the threads: threads in the ExecutorService are always named pool-n-thread-m. If you really want to change that, you need to implement your own ThreadFactory in the ThreadPoolExecutor.

Moreover, passing Thread objects to a ExecutorService is confusing since you may think the threads will be started whereas only their method run() will be executed.

Or maybe, you just need something like that:

executor.execute(new Runnable() {
	private final String name ="sendMail process";
	
	@Override
	public void run() {
		try {
			sendMailClosure(session, email);
		} catch (Exception e) {
			LOGGER.error("could not send email {}", email, e);
		}
	}

	@Override
	public String toString() {
		return name;
	}
});

Would it be ok for you this way?

[bbottema]

Sounds like it would do the trick. I'll check it out when I get home from vacation. In the meantime I can merge it using my phone. Good work!

[amanteaux]

Thank you for your time, enjoy your holiday!

[bbottema]

I don't think the extra try...catch is needed there. Timeout exceptions are already caught as MessagingException and rethrown as generic errors by Simple Java Mail.

[amanteaux]

Actually the extra try...catch is needed to show information about the e-mail that could not be sent.
Currently, when there is an exception in the async sending process, the exception is printed by the JVM. That is useful because we want to know why the e-mail were not sent, however it raises 2 issues:

• since the exception is raw printed, we do not have any time information about the exception,
• we do not have any information about the e-mail that weren't sent.

The goal of the try catch is to address these 2 issues.

I have attached the log lines with and without the try catch.
without_try_catch.txt
with_try_catch.txt

[bbottema]

Ahh I see. I'll make sure the extra details will be clear from the output.

[amanteaux]

Thank you!