Skip to content

Conversation

jmazon
Copy link
Contributor

@jmazon jmazon commented Oct 4, 2013

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.

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
Copy link
Owner

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
Copy link
Owner

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 bigpresh merged commit 85b631d into bigpresh:master Dec 22, 2013
@bigpresh
Copy link
Owner

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 jmazon deleted the patch-1 branch December 22, 2013 22:13
@jmazon
Copy link
Contributor Author

jmazon commented Jan 14, 2014

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
Copy link
Owner

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

@jmazon
Copy link
Contributor Author

jmazon commented Jan 17, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants