Fix GH-22631: ext/mysqli: fix corruption of num_active_persistent when using real_connect#22753
Open
bavis-m wants to merge 1 commit into
Open
Fix GH-22631: ext/mysqli: fix corruption of num_active_persistent when using real_connect#22753bavis-m wants to merge 1 commit into
bavis-m wants to merge 1 commit into
Conversation
…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 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 phpGH-22631
|
|
||
| /* store persistent connection */ | ||
| if (persistent && (new_connection || is_real_connect)) { | ||
| if (persistent && (new_connection || is_real_connect) && !from_pool) { |
Member
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.
Author
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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