search

gabordemooij / redbean

ORM layer that creates models, config and database on the fly
https://www.redbeanphp.com
2.31k stars 280 forks source link

Testing connection with R::testConnection() #933

Closed marios88 closed 1 year ago

marios88 commented 1 year ago

For context, i want to make a long running worker process that must periodically check that the database is still connected.

To my surprise R::testConnection() does not actually test the connection but only checks its initialized.

Do you think its in the scope of the function to actually check if the connection is alive? like querying SELECT 1

Lynesth commented 1 year ago

Hey there.

I'm... actually confused about that since I recall doing exactly that some time ago, but I'm a bit too tired to look into what happened there. Maybe @gabordemooij will tell us.

Related PR: https://github.com/gabordemooij/redbean/pull/673/files

gabordemooij commented 1 year ago

Looking into this...

gabordemooij commented 1 year ago

I am not sure why/how it has been removed, but I do see it might have some issues:

  1. It's not fully backward compatible (in a subtle way), i.e. it actually fires a query now
  2. It might return TRUE while the 2nd attempt fails
  3. Also the SQL might fail for a different reason and it will return FALSE while the connection is alive

My suggestion would be to add a new function like testConnectionSQL, that tests the connection with a query and addresses the issues above.

Lynesth commented 1 year ago

Hey, thanks for taking the time to look into this :D

My opinion about the current testConnection() is that it's pretty much useless as it's just a connect() followed by a isConnected().

And connect() does nothing if it was called previously at any point (unless connection was manually closed of course) since it sets $isConnected to true and then relies on that.

And since a simple R::setup( ... ) will call connect(), the only time testConnection() actually tests the connection is if it's called right after close(). And that's also the only time it would return something different than isConnected().

That was the whole point behind that PR 5 years ago.

Now to talk about the three points you raise:

  1. Yes it fires a SELECT 1 by default, I'm not sure how this breaks backward compatibility though, but I'm probably missing something, could you elaborate on this?
  2. If you're talking about the automatic reconnection, I believe we would get an exception from connect() if that fails so it should be fine?
  3. You mean, in case people modify the $sql argument for something other than SELECT 1 and that something being invalid SQL? Then yes, that's true. If they don't have $autoreconnect set then it will return false, but that sounds more like a user issue in that case (unless I misunderstood your 3rd point).*

*I made the SQL customizable just for the sake of it but SELECT 1 should be good for almost everyone.

gabordemooij commented 1 year ago
  1. Actual behavior might differ, point is, people might rely on the old behavior for some reason.
  2. I guess it's fine...
  3. Yes, this would be a user error, but still would have been nice if we could "catch" that

Anyway, I propose a slightly changed PR:

  1. Same as yours but under different name so as not to break any BC
  2. In case of auto reconnect, the function calls itself with $autoreconnect = FALSE so you get a correct boolean answer even if reconnect fails somehow without exception

https://github.com/gabordemooij/redbean/pull/934