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

Save a reference to the new handle when reconnecting #44

Closed jmazon closed 10 years ago

jmazon commented 10 years ago

After a database disconnect, the current version of the code reconnects just fine, but doesn't seem to actually save the reference, so the next call will just find a handle that looks just as inactive as before, and attempt reconnection again.

This is not only wasteful in terms of reconnection attempts, it actually introduces bugs when the software around it depends on the session being stable between two calls. (which was my case)

The proposed patch fixes that in a way that's good enough for me (TM). I have no idea how to reliably test for this.

bigpresh commented 10 years ago

Sorry for leaving this one so long - was meaning to come back to it when I had enough time to figure out how to effectively test it. Looking at it again, I'm pretty confident it will work fine.

bigpresh commented 10 years ago

I'm thinking a check similar to the '/handles_cached route in the test app, which fetches a handle, then calls disconnect on it, then fetches two more handles and check that we get a new handle (but the same new handle twice).

e.g.

get '/handles_cached_after_reconnect' => sub {
    my $old_handle = database();
    $old_handle->disconnect;
    my $new_handle = database();
    if ($old_handle ne $new_handle && $new_handle eq database()) {
        return "New handle cached after reconnect";
    }
};
bigpresh commented 10 years ago

Added a test as per my idea above, which failed before applying your patch and succeeds now - so I'm happy that's fine.

Thanks for your contribution, sorry for the delay getting it merged, and best wishes for a wonderful Christmas!

jmazon commented 10 years ago

Hi, thanks for merging.

After a few weeks of production use, it seems like it's not as Good Enough For Me as I thought, though I haven't been able to reproduce reliably yet.

I added some additional informational logging around the reconnection code, in hope of pinning down the problem more precicely. I hope I'll have good news to share soon.

Happy new year!

bigpresh commented 10 years ago

Hmm, sorry to hear it's still giving you problems - look forward to more info - good luck :)

jmazon commented 10 years ago

I'm not going to state it solved as boldly as before, but it seems stable now.

The core problem was that I messed up my Carton and upgraded Dacner::Plugin::Database and not Dancer::Plugin::Database::Core. Haha, oops.

The smaller issue is that it doesn't keep the reconnection timestamp until next step. I'm opening a PR for that.