bigpresh / Dancer-Plugin-Database

Dancer::Plugin::Database - easy database support for Dancer applications
http://search.cpan.org/dist/Dancer-Plugin-Database
37 stars 36 forks source link

Applying settings to database() at runtime does not reuse database connections #75

Open yahermann opened 8 years ago

yahermann commented 8 years ago

Per documentation, using database() in this way:

my $dbh = database({ driver => 'SQLite', database => $filename });

causes a brand new connection to be created each time, and old connections don't get disconnected, so the max gets hits quickly.

The problem appears to be in Database.pm, lines 52-54:

if (ref $arg eq 'HASH') {
    $handle_key = $arg;
    $conn_details = _merge_settings($arg, $settings, $logger);
 } else {

Even though $arg may be the same hash, i.e., { database => "mydb", username="myuser" }, unless special precautions are taken, generating it during runtime creates a different anonymous hashref each time, therefore when used as a key, Perl treats them each as different keys. Since they're different keys, a new connection gets created with each call, and old connections/handles are not reused.

ambs commented 8 years ago

Not sure if there is a way to create an unique hashref. An option would be to serialize (sorting keys) and create md5 or something as key. Other option would be to add a name => foo option, and reuse when name is the same.

@bigpresh , thoughts?

yahermann commented 8 years ago

For anyone that runs into this problem, I kludged my way around this issue by setting the database definition hash globally:

use strict;
use Dancer ':syntax';
use Dancer::Plugin::Database;
....
my $db = { database => 'mydb', username => 'myuser', ... };
....
return -1;

and then anywhere a database handle is needed,

my $dbh = database($db);

The key is to avoid using this feature as shown in the documentation; otherwise, database handles will not be reused and you will quickly hit your max_connections threshold.

ambs commented 8 years ago

Nice solution, @yahermann. Thanks for sharing!

ambs commented 8 years ago

Let me reopen the ticket. We need to either correct the behavior or to document your approach and warn users.

yahermann commented 8 years ago

Fair enough :-)

bigpresh commented 8 years ago

Hmm, sorry I managed to miss this one. Yes, agreed, we ought to work out a fix - serialising the hashref contents to generate a key sounds like the best option which would DWYM.

yahermann commented 8 years ago

I think serializing the hashref would work to generate a unique scalar key, as long as the hashref keys AND values AND subkeys/subvalues recursively (e.g. dbi_params => { ... } ) are all part of the scalar key, and sorted by each key/subkey within the hashref.

There must be a Perl module that does this in a reliable and consistent way (and hopefully already existing in the dependency chain). Otherwise you would DWYM intermittently, which is much worse!

Another approach, and one that I think I prefer: Require the user to manually specify a scalar key when using this option, explicitly for the purpose of reusing database handles.

yahermann commented 8 years ago

I ran into another ugly problem using database() with a hashref, that is contrary to the documentation.

It appears the connection_check_threshold, which according to the documentation is supposed to default to 30 (seconds) if not specified, is actually not set at all when using database() with a hashref. It must be specified in the passed hashref. Failing to do so results in eventual and consistent 'mysql server has gone away' errors unless the database is used more frequently than whatever is specified in mysql wait_timeout (which defaults to 28,800 seconds, or 8 hours).

bigpresh commented 7 years ago

I finally had a chance to hack on this, and got part-way, but one fun problem: the settings may contain coderefs, e.g. HandleError, whatever serialisation form is used needs to understand that. It will probably have to just ignore coderefs, so there's the risk that, if all other arguments are the same but the coderef is different, you still get the cached handle; that sounds reasonable enough if documented. I'm going to head to bed in a moment, so I'll poke at this again as soon as time permits.