pdo_dblib: Do not reuse dead connections#21681
Conversation
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.
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!
| if (dbdead(H->link)) | ||
| return FAILURE; |
There was a problem hiding this comment.
nits:
https://github.com/php/php-src/blob/master/CODING_STANDARDS.md#syntax-and-indentation
| if (dbdead(H->link)) | |
| return FAILURE; | |
| if (dbdead(H->link)) { | |
| return FAILURE; | |
| } |
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.