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 a custom ExecutorService to be set for each pool #113

Merged
merged 1 commit into from
Jul 19, 2014
Merged

Allow a custom ExecutorService to be set for each pool #113

merged 1 commit into from
Jul 19, 2014

Conversation

keir-nellyer
Copy link
Contributor

This allows for HikariCP to be used when a SecurityManager is in place, preventing/warning of the creation of threads not using the applications main ExecutorService. One such application is BungeeCord, you can see it's security manager here. Currently it's only warning of the action.

I believe Android also has a similar limitation in place.

Users of the library may also want to change the ExecutorService so that they can reuse it throughout their project.

@brettwooldridge
Copy link
Owner

What about the housekeeping timer thread? Wouldn't that issue a similar warning?

@brettwooldridge
Copy link
Owner

It also appears that BungeeCord will not issue a warning if the Threads are created through the GroupedThreadFactory: BungeeSecurityManager:58

Using an external Executor could be bad news for the pool, causing lock-ups etc. because we are at the whim of whoever configures the Executor. For example an Executor with a CallerRunsPolicy.

In lieu of specifying an Executor, I would prefer to allow the specification of the ThreadFactory that is used by the Executor that HikariCP instantiates.

@keir-nellyer
Copy link
Contributor Author

@brettwooldridge Okay, that was definitely something I overlooked and appears to be a much better solution, implementing and testing now.

@keir-nellyer
Copy link
Contributor Author

I've implemented specifying a custom thread factory, however I don't think there's any way to control how the Timer class creates it's thread without creating our own (we can't even extend it to sort the issue).

A way around this would be to switch over to using a ScheduledThreadPoolExecutor in place of the timer and just have a poolSize of 1 (or more, although I think 1 is enough as that is what a Timer effectively was). I want to make sure though that the ScheduledThreadPoolExecutor won't add any extra overhead having the pooling features.

@keir-nellyer
Copy link
Contributor Author

Okay, I've figured a way round the issue with Timers and now using a ScheduledThreadPoolExecutor too, I did change some method parameters in one of the interfaces, I'm not sure if any applications using HikariCP will be using that interface. When I commit it I'll show you what I mean.

If it does break anything I can probably add a deprecated method that will prevent any applications breaking due to the change.

@keir-nellyer
Copy link
Contributor Author

Darn, will try get rid of that merge commit. Nevermind, fixed. GitHub makes me look crazy by removing the commit messages that were there.

…Factory will be used to create all threads used in that pool
@keir-nellyer
Copy link
Contributor Author

Updated to changes suggested.

@keir-nellyer
Copy link
Contributor Author

The Travis build failed due to not being able to connect to the database, doesn't seem like an issue with this PR.

brettwooldridge added a commit that referenced this pull request Jul 19, 2014
Allow a custom ExecutorService to be set for each pool
@brettwooldridge brettwooldridge merged commit e54fe43 into brettwooldridge:dev Jul 19, 2014
@brettwooldridge
Copy link
Owner

Thanks for the contribution!

@keir-nellyer
Copy link
Contributor Author

Thanks for merging!

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