Allow threads to timeout in the thread pool by amanteaux · Pull Request #206 · bbottema/simple-java-mail
Conversation
As you asked, I did not go far in the config wiring. But if this solution is ok, I can do it if you want.
Currently, I only verified on a real project that the sending thread expired correctly after 1 second. But to check that threads are correctly expiring, it is possible to write a unit test. I can do it, but I prefer first to validate this!
About the exception bubbling up in the ThreadPoolExecutor and not being logged through SLF4J should I create a separate issue?
No actually the error should be logged in 5.1.6. In 6.0.0 (develop) it is allowed to bubble up as it is now. I'll reapply that change. Or you can do it, no problem.
Currently, I only verified on a real project that the sending thread expired correctly after 1 second
So I'm seeing 1 preconfigured in theOperationalConfigand MILLISECONDS on the executor. I suppose
you tested it with 1000 instead of 1?
No actually the error should be logged in 5.1.6. In 6.0.0 (develop) it is allowed to bubble up as it is now. I'll reapply that change. Or you can do it, no problem.
Ok thank you, I will reapply the change!
you tested it with 1000 instead of 1?
My mistake... I tested with 1 millisecond. Actually I first used milliseconds, but for that it would have been better to use a Long duration instead of an Integer one. And to use a Long, more work would have been necessary in the configuration part, since no method currently exists to parse a Long value.
Anyway, I can make this pull request better (unit testing for timeout, correct seconds or milliseconds timeout, configuration wiring), but before I spend too much time on this, I would prefer to be sure you are ok with what I will be doing.
Do these changes seem acceptable for you or do you think the value is too low for the added complexity?
I had some difficulty deciding whether we're solving any problem here. I still don't think so. However, using a custom ThreadPoolExecutor, we can enable users to fine tune the threading policy of Simple Java Mail, which is pretty useful.
I propose making both core and max threads configurable as well as the timeout. However, I would like to solve the #204 bug in 5.1.6 and expose the threading parameters in 6.0.0 as new functionality. The bug was fixed by switching from .isTerminated() to .isShutdown(). All the other changes can be ported to a branch of develop.
Sidenote, I'm still working on understanding why we need core threads at all when we're allowing them to time-out anyway as not to block the JVM from shutting down. Have an SO question out to help me with that (more questions in the comments there): https://stackoverflow.com/questions/55879100/with-threadpoolexecutor-what-is-the-difference-between-allowcorethreadtimeout-a
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters