Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow threads to timeout in the thread pool #206

Conversation

amanteaux
Copy link
Contributor

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?

@bbottema
Copy link
Owner

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.

@bbottema
Copy link
Owner

bbottema commented Apr 26, 2019

Currently, I only verified on a real project that the sending thread expired correctly after 1 second
So I'm seeing 1 preconfigured in the OperationalConfig and MILLISECONDS on the executor. I suppose

you tested it with 1000 instead of 1?

@amanteaux
Copy link
Contributor Author

amanteaux commented Apr 26, 2019

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?

@bbottema bbottema merged commit 4032490 into bbottema:204-fix-race-condition-improve-threading Apr 27, 2019
@bbottema
Copy link
Owner

bbottema commented Apr 27, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants