-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add idle check to connection cleaner and rework when it runs #60
Conversation
if let Some(lifetime) = max_idle_lifetime { | ||
match internals.config.max_idle_lifetime { | ||
Some(prev) if lifetime < prev && internals.cleaner_ch.is_some() => { | ||
// FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you had in mind for the result here @importcjj
Returning a result would break the API for set_conn_max_lifetime
Hum there seems be a race condition (most likely unrelated to the current changes) in https://travis-ci.com/github/importcjj/mobc/jobs/518297137#L1515 |
There should probably be a way to not lock the whole internal for the connection cleaner to run since that can take time, ideas are appreciated. |
Great PR, when will it be available ? |
I mean it all depends on @importcjj |
Sorry for late, I will check it soon. |
I think it is necessary to limit the execution time and the number of connections traversed when cleaning connections. For example:
|
We could do two things
Also I checked the sqlx pool and they managed to removed the lock by using a lock free vector like structure. Might be worth a eventually. |
Actually, I have long wanted to rebuild this connection pool. I want to remove all the locks and use a single-thread events loop just like Redis. But I doubt how many improvements the new architecture can give, it needs to be tested. |
Hello is connection pool subject an already closed subject in the tech world ? I seems to be weird to reinvent the wheel no ? |
If you can bring some things new, it's worth doing. |
Nope, but waiting for it :P |
Unsure what are the next steps here, if we merge this PR it will behave better in general but will lock the mutex for a long time. |
Have you take a look here : https://github.com/vincit/tarn.js ? (what do you think about this one ?) |
I wrote a simple prototype using the event loop, and with a simple test, I think it has a much better performance than the current one. The pseudo-code below is the core code of the prototype:
|
Interesting, the challenge will be to ensure the loop is never blocked for long. |
I have added you to the collaborators of this repo, you can submit your code to the branch 'v2-dev'. |
Hi, it's been a while, I wonder what's missing from this PR? |
@jkomyno Its been a very long time since I have been out of prisma / mobc and the code I wrote at the time probably was not the best. I am unsure of the state of mobc but at the time it was agreed that it needed a full rewrite and I suggested prisma looked at https://github.com/bikeshedder/deadpool as I worked with the dev to make sure it had all the requirements needed and a better code base. I see some else suggested that to you more recently prisma/prisma#19215. |
See my above comment |
Fixes #58
So this is somewhat of a big change:
I added tests to reflect the new behaviour but let me know if you want more.