Fix GH-22631: ext/mysqli: fix corruption of num_active_persistent when using real_connect#22753
Fix GH-22631: ext/mysqli: fix corruption of num_active_persistent when using real_connect#22753bavis-m wants to merge 1 commit into
Conversation
|
|
||
| /* store persistent connection */ | ||
| if (persistent && (new_connection || is_real_connect)) { | ||
| if (persistent && (new_connection || is_real_connect) && !from_pool) { |
There was a problem hiding this comment.
Is there really no way to simplify this? This is getting difficult to read already. Also, it would be nice if you could add a test for this.
There was a problem hiding this comment.
I agree, what do you think about just removing the || is_real_connect (and discarding the from_pool flag)? That was my original plan, but after some discussion on 22631 I changed that. I'm not sure though that there is a case where that || is_real_connect is needed: either new_connection should be set (we seem to try and close any existing connections), or we should have gotten a cached one in which case we already incremented the counter.
0ff9b75 to
8db991c
Compare
…onnect() Fix phpGH-22631: ext/mysqli: mysqli double increments num_active_persistent... In mysqli_common_connect(), when is_real_connect is set and a persistent connection is requested, both the persistent connection branch and the normal function flow (in the end: label) increment the num_active_persistent counter. num_active_persistent is also decremented once as a result of cleaning up old persistent connection state, but this still results in the counter growing by 1 every time we reuse an existing connection, and becoming invalid. This can cause later non-cached persistent connection attempts to fail the max_persistent limit check, depsite there not actually being that many persistent connections. This commit unifies the 2 places num_active_persistent could be incremented: - the persistent branch, before jumping to end: - the main branch, where it was incremented f persistent was true and new_connection was true Now, we just increment in the end: block if persistent is true, and new_connection is no longer needed. Fixes phpGH-22631
8db991c to
ea618a9
Compare
|
@kamil-tekiela I've tried another version here where we don't need the |
|
This does look better, but I would still love to have a test for this bug. |
2 participants
Fix GH-22631: ext/mysqli: mysqli double increments num_active_persistent...
In mysqli_common_connect(), when is_real_connect is set and a persistent connection is requested, both the persistent connection branch and the normal function flow (in the end: label) increment the num_active_persistent counter. num_active_persistent is also decremented once as a result of cleaning up old persistent connection state, but this still results in the counter growing by 1 every time we reuse an existing connection, and becoming invalid. This can cause later non-cached persistent connection attempts to fail the max_persistent limit check, depsite there not actually being that many persistent connections.
This commit adds a flag from_pool which is set when a connection is found in the persistent connection pool, and which prevents the counter increment in the end: label from firing.
Fixes GH-22631