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

Rework pool lock logic to handle new connections context switches #109

Merged
merged 3 commits into from Sep 12, 2019

Conversation

@bcardiff
Copy link
Member

commented Aug 20, 2019

Fixes #77

I was able to make a spec to repro the issue in crystal-lang/crystal-mysql@325493b#diff-f41a8330fda8a5ae668e7e17638aef40R40-R57

A couple of things I considered during this PR:

  • go's sql package added a SetConnMaxLifetime that would be good to have in order to limit the timespan of the whole connection. It's a good policy.
  • When releasing a connection and if the max idle connection was reached, we could choose where to drop one in the idle pool rather than the one that is been released in order to keep the younger one.
  • The checkout_some might be redundant. Maybe is just enough to reuse the statement and allow any connection to be used. If we do this, the idle connections could be kept in a Channel, as the message that is sent. But this will also require for the buffered channel to have a (theoretical) unlimited max capacity.

For now, this PR should fix the issue but there are some ideas to improve the logic.

@bcardiff bcardiff changed the title Rework pool lock logic to handle new connections context switched Rework pool lock logic to handle new connections context switches Aug 20, 2019
@Xosmond

This comment has been minimized.

Copy link

commented Aug 20, 2019

@bcardiff Hey, great to know you worked on this PR, I will try it tomorrow to see it under pressure

@bcardiff bcardiff force-pushed the pool-lock branch from 7a9de8a to 13066f9 Aug 23, 2019
src/db/pool.cr Outdated Show resolved Hide resolved
@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

@Xosmond were you able to check if this PR works for you?

@bcardiff bcardiff force-pushed the pool-lock branch from 27dc656 to 65c0705 Sep 11, 2019
@bcardiff bcardiff force-pushed the pool-lock branch from 65c0705 to afad416 Sep 11, 2019
@bcardiff bcardiff merged commit ff5c326 into master Sep 12, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@straight-shoota straight-shoota deleted the pool-lock branch Sep 12, 2019
src/db/pool.cr Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.