ext/pdo_dblib: Added dblib_handle_check_liveness handler#21681
ext/pdo_dblib: Added dblib_handle_check_liveness handler#21681freddy77 wants to merge 1 commit intophp:masterfrom
dblib_handle_check_liveness handler#21681Conversation
SakiTakamachi
left a comment
SakiTakamachi
left a comment
There was a problem hiding this comment.
Sorry for the delayed review.
This makes a lot of sense.
Regarding tests, I’ve done something similar in pdo_firebird before. In that case, I wrote a test to ensure that check_liveness is not broken and that the same connection is reused when using persistent connections.
https://github.com/php/php-src/blob/master/ext/pdo_firebird/tests/persistent_connect.phpt
For SQL Server or Sybase, you can obtain the connection ID with SELECT @@SPID, so it should be possible to create a test similar to the one for Firebird.
If this would be a burden for you, I can also add the test myself.
Anyway, thank you for this change!
In case of persistent connection it was not checked if the connection was still alive always assuming it was. If the connection was broken this caused PHP to reuse the broken connection over and over. dbdead function is supported by all dblib implementation (MS, Sybase, FreeTDS). Change tested manually, see FreeTDS/freetds#711 (comment) Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
SakiTakamachi
left a comment
SakiTakamachi
left a comment
There was a problem hiding this comment.
Although I created a test, it turned out to be largely meaningless for SQL Server and Sybase. I’ve decided the test is unnecessary and will proceed to merge as is.
Thank you!
SakiTakamachi
changed the title
dblib_handle_check_liveness handler
|
Thank you. |
2 participants
In case of persistent connection it was not checked if the connection was still alive always assuming it was. If the connection was broken this caused PHP to reuse the broken connection over and over.
dbdeadfunction is supported by all dblib implementation (MS, Sybase, FreeTDS).Change tested manually, see FreeTDS/freetds#711 (comment)
Feel free to update comment or code.
As the comment above I didn't test the change myself.
I don't see an easy way to write a test for this change.