Add a method to get the underlying ThreadFactory for use with external libraries #1117
Conversation
|
You could replace the |
|
Also, other classes should not be able to create Unsafe instances. |
|
@yawkat Thanks for the tip, I never knew you could do that but I assumed there must be a way. And I'll make the constructor private. |
|
Thanks! |
|
@yawkat I guess whichever one gets pulled first, the other updates their code accordingly. Also fixed. |
|
@yawkat Actually it seems your PR is not up to date with the upstream repo,the getExecutorService() (not in Unsafe) was not added by me, it's already in the upstream repo. Your PR adds it as if it never existed which may cause merge issues. |
|
Nah, I didn't add it either, it's just a diff derp. |
|
@yawkat Ah okay. |
|
My PR on HikariCP got accepted, just waiting on this now :) |
…l libraries This method was added in the sub-class "Unsafe" in Plugin. This follows the Unsafe pattern used in other parts of the project. getExecutorService() was also moved into the Unsafe class and the old method simply forwards to the one in the Unsafe class, this prevents plugin breakages.
|
Updated to use md_5 styled formatting in 2 places I missed. |
|
@md-5 Anything preventing this from being accepted? |
|
Yes, I don't see why its needed given that there is the solution of instantiating within a callable. Threads are BAD in a plugin environment and should be avoided at all costs. Also if you were PRing HikariCP, why wouldn't you add an executor instead of a thread group. |
|
@md-5 I did originally use an Executor however it was decided that would be a bad idea (see brettwooldridge/HikariCP#113 (comment)). Instantiating within a Callable works, however it seems like more of a workaround. Passing in a ThreadGroup would allow you to continue to keep track of the threads (whereas creating them within a Callable would not). |
|
This is what the developer of HikariCP said (brettwooldridge) "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." - link Threads are often extremely useful and quite often unavoidable. They are amazing when used correctly. At least if you accept the PR, and developers make use of it, you can still track their threads and shut them down when needed. Whereas if it is kept as is, developers are simply going to bypass it by creating the thread in an async scheduler task in which case you cannot track the thread and shut it down when needed. I make use of threads in a private project I work on. We use HikariCP for MySQL connection pooling which makes use of threads and we also use Netty (which also uses threading) for server-to-server communication. A ThreadFactory can be passed to HikariCP and an ExecutorService can be passed to Netty (why they went with an ExecutorService rather than a ThreadFactory, I'm not sure, as the issues mentioned above might occur). Basically we're warning developers by keeping these methods in an unsafe sub-class, if they choose to abuse it, that's their fault. |
|
My point still stands, I won't be adding this. Since every man and his dog seems to want their own SQL stuff I would say a better solution is to expose access to a variety of databases from within bungee. There shouldn't be a reason why you specifically need your super Dooper faster than all the rest connection pool because most of that is rubbish (not referring to any particular one) |
|
@md-5 Connection pooling is definitely faster, and HikariCP is basically one of the only active connection pooling libraries out there anyway. One connection is definitely not going to be enough for the amount of traffic I am anticipating, so that leads us to opening multiple connections. Keeping a connection open is always going to be faster than constantly opening and closing. Which basically brings us to writing our own connection pooling library, simply so that it can be compatible with BungeeCord, and quite frankly my library is not going to be faster than existing libraries by developers with more experience in that area. It's also reinventing the wheel. You yourself use a ThreadPoolExecutor (an example of pooling), why do this instead of just opening threads? Because it's faster. Sorry, my comments seems really hostile, I honestly don't want to get on your wrong side, I'm just arguing my point. |
Hikari, bonecp and c3p0 are all compatible with bungee as is. Use the scheduler and a callable like Tux does. My point is if we were to include SQL APIs into bungee it would be ridiculous to say that "bungee uses bonecp I want to use hikari". At the end of the day a connection pool is very simplistic and there is very little difference between them. Not once did I say you shouldn't use pooling. |
|
Okay, I just find that kinda hacky. And this PR isn't specific to anything,
|
|
The idea is to discourage creation of threads. |
|
That's why it's in a sub class named Unsafe, still discourages it but And if your discouraging the use of threads, why expose the
|
|
Because they allow us to have finer grained control where possible. |
|
To further that, the API should be sufficient that you don't need additional libraries for common use cases such as SQL. |
|
@md-5 If you give us access to the ThreadFactory though, it will still allow you to control the threads. However if we have to use a workaround by created threads in an async task, this doesn't allow you to control the threads. "To further that, the API should be sufficient that you don't need additional libraries for common use cases such as SQL." Should but isn't, and Bukkit attempted to implement a common MySQL system but nobody uses it. |
That's because it's a silly ORM and not just a way to get a connection. |
|
@md-5 What if I modified this PR to return a generic ThreadFactory instead of a GroupedThreadFactory. Also until a sufficient database system is added and supports pooling, I believe we should have some sort of api method that allows us to get the ThreadFactory so we can do it ourselves in the meantime. If you have any suggestions to make which would make you consider accepting this, please let me know. |
|
You can already do the following to make your stuff work:
All the thread factory does is specify a ThreadGroup. Since you can already get the thread group from an async task + callable pair, I do not see why you need an additional method to do so. |
This method was added in the sub-class "Unsafe" in Plugin. This follows the Unsafe pattern used in other parts of the project.
getExecutorService() was also moved into the Unsafe class and the old method simply forwards to the one in the Unsafe class, this prevents plugin breakages.
This PR here (which should hopefully soon be accepted) shows how these new API methods might be used with external libraries. This allows BungeeCord to continue to keep track of threads created by plugins.
One thing I am slightly unsure about is if the shutdown call here will shutdown threads that are assigned to the same group as that ExecutorService, but weren't created by that ExecutorService.