Open database connection in mutex

[matthewmcgarvey]

Adding Lucky to TechEmpower benchmarks showed us that we were hitting Postgres' connection limit. The pool limit we set for our app was 56 per connection. Starting 10 instances of the app should have only opened up 560 connections, but we started hitting the postgres configuration's 2000 connection limit! Adding in some puts debugging showed that this Avram::Database#db method was getting called and going past the memoized value more than we were expecting. The theory is that an instance of the app was handling requests so fast that another request tried to get a database connection before the previous one had fully opened one and set the class variable. The load test was going so fast that it was sending a ton of requests into this situation, thus hitting the connection limit.

By adding a mutex lock around it, we make sure that situation doesn't happen by forcing only one thread (request) to be able to open a connection at a time. We do the class variable check inside the mutex because requests could be queued up at the mutex having already checked the class variable while it was getting opened.

I don't expect many apps to have experienced issues with this as the app has to have experienced heavy load before the db variable was set but this was definitely a bug. Maybe there is a better way to solve it?

Are other ORM's affected by this?

Granite - doesn't seem to be as they require registering a connection in the app start process https://docs.amberframework.org/granite/docs/docs#register-a-connection

Crecto - they memoize the connection and lazily call it exactly as we do. https://github.com/Crecto/crecto/blob/master/src/crecto/repo/config.cr

Jennifer.cr - also memoizes https://github.com/imdrasil/jennifer.cr/blob/22bbaa466856594ef601589faae25320c8aa3f20/src/jennifer/adapter/base.cr#L39

[jwoertink]

I don't really understand what a Mutex is/does, but your explanation makes sense. We can probably just merge this in, and then get it in the hands of people before we do a release. There's a lot of funky stuff going on in Avram right now that we may need a few PRs to touch things up. So if you're cool with this, feel free to merge in and I can try to get it in my apps sometime soon.

[jkthorne]

I am curious how are you testing this. Is seems like a performance change and there should be a benchmark that should show this is a benifit.

[matthewmcgarvey]

@wontruefree You're right. This is difficult to test, but you can check out the TravisCI runs on FrameworkBenchmarks. It has the failures we were diagnosing and the monkey patch that led to this PR TechEmpower/FrameworkBenchmarks#6086