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

$dbh->ping may raise exception, causing crash. #65

Closed jamesrusso closed 8 years ago

jamesrusso commented 9 years ago

Hello,

I am working with DBI::SQLAnywhere and it's ping method simply calls $dbh->prepare("select 1"). If the database connection has gone away due to timeout, etc, then this will raise an exception during the ping call and will crash that request. This is not the desired result. We should try and re-connect and crash if re-connection fails.

I supposed it really could be done either location, but I see no reason why it shouldn't be done in Dancer-Plugin-Database since it would be beneficial to other DBIs.

thoughts?

-jr

bigpresh commented 9 years ago

On Wednesday 13 May 2015 11:15:58 James Russo wrote:

Hello,

I am working with DBI::SQLAnywhere and it's ping method simply calls $dbh->prepare("select 1"). If the database connection has gone away due to timeout, etc, then this will raise an exception [...]

That seems like odd behaviour, from a method explicitly designed to be called to find out if the database connection is still responsive; raising an exception if not seems completely insane behaviour there, to me.

I checked the DBI documentation to see if it offers guidance on what a ping() method should do, but it is very vague:

"The current default implementation always returns true without actually doing anything. Actually, it returns "0 but true" which is true but zero. That way you can tell if the return value is genuine or just the default. Drivers should override this method with one that does the right thing for their type of database."

I would prefer to see this fixed in DBD::SQLAnywhere instead, but may consider catching exceptions within D::P::D for convenience in case any other DBI drivers act similarly.

I've raised a ticket against DBD::SQLAnywhere for this: https://rt.cpan.org/Ticket/Display.html?id=104410

If the maintainers of DBD::SQLAnywhere are willing to fix it upstream, then result, problem solved; if not, I'll work round it in D::P::D for you :)

jamesrusso commented 9 years ago

Ok, sounds good. I threw a quick pull request up for you to consider. I'll follow up with the SQLAnywhere guys.

-jr

jamesrusso commented 9 years ago

I haven't completely tracked it down, but I think my problem is that both an exception is raised and then it is also printed. So, even when I eval and catch the exception in the DBD::SQLAnywhere module, the request still crashes because the error was printed? Still need to do more tracking down. After spending some more time on this, I do think the correct place to fix this is in the DBD::SQLAnywhere module.

jamesrusso commented 9 years ago

So, this is not that simple. The error handling in DBI is executed after the return from the dispatched function (ie: ping). So, you really cannot eval the prepare statement in the ping method because the prepare statement will just save the error until the return of the ping method. The only thing I've found to work for me is to disable RaiseError on the DBH which I really don't want to do for my entire application.

Also interesting is that doing a local $dbh->{RaiseError} = 0 in the ping method still causes does the die due to the prepare failure, not sure why this isn't working..

ambs commented 8 years ago

Merged your solution, for now. If needed, we will readdress this problem. Thanks.