Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: deallocate mysqli prepared statement #6681

Conversation

petermein
Copy link
Contributor

@petermein petermein commented Jan 3, 2025

Long running processes might hit the max_prepared_stmt_count due not deallocating the statement correctly.

Q A
Type bug
Fixed issues -

Summary

For postgres there was an implementation to deallocate prepared statements to prevent hitting a any limits. This was not the case for the mysqli drivers. Which can suffer from the same problem.

In this PR a mysqli_stmt_close is added to properly cleanup any open prepared statement.

A similar approach was implemented for PostgreSQL in this PR #5893

@petermein petermein changed the title bugfix: deallocate mysqli prepared statement WIP: bugfix: deallocate mysqli prepared statement Jan 3, 2025
@petermein petermein force-pushed the peter.mein/bugfix-mysqli-dealloc-prepared-statement branch 3 times, most recently from c3b7675 to d92e954 Compare January 3, 2025 13:54
@petermein petermein changed the title WIP: bugfix: deallocate mysqli prepared statement bugfix: deallocate mysqli prepared statement Jan 3, 2025
@petermein petermein force-pushed the peter.mein/bugfix-mysqli-dealloc-prepared-statement branch 2 times, most recently from cb120c9 to ea38c64 Compare January 3, 2025 15:04
@derrabus
Copy link
Member

derrabus commented Jan 5, 2025

Thank you. Can you try to rebase your bugfix onto the 3.9.x branch?

@petermein petermein changed the base branch from 4.2.x to 3.9.x January 6, 2025 07:56
@petermein petermein force-pushed the peter.mein/bugfix-mysqli-dealloc-prepared-statement branch 5 times, most recently from a5e719b to d41af03 Compare January 6, 2025 08:55
@petermein
Copy link
Contributor Author

Thank you. Can you try to rebase your bugfix onto the 3.9.x branch?

Re-based the PR to point to 3.9.x and changed to code to not include any still unsupported language features (property promotion and attributes).

Please have a look if you like the wording of the comments.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the PHPCS report.

src/Driver/Mysqli/Result.php Outdated Show resolved Hide resolved
src/Driver/Mysqli/Result.php Outdated Show resolved Hide resolved
src/Driver/Mysqli/Result.php Outdated Show resolved Hide resolved
@derrabus derrabus added this to the 3.9.4 milestone Jan 6, 2025
@petermein petermein force-pushed the peter.mein/bugfix-mysqli-dealloc-prepared-statement branch 3 times, most recently from 1a186f3 to 8b453c7 Compare January 6, 2025 11:49
@derrabus
Copy link
Member

derrabus commented Jan 6, 2025

Can you look into the test failures on PHP 7.4?

@petermein petermein force-pushed the peter.mein/bugfix-mysqli-dealloc-prepared-statement branch from 8b453c7 to 8fd19f9 Compare January 6, 2025 13:38
@petermein
Copy link
Contributor Author

petermein commented Jan 6, 2025

Can you look into the test failures on PHP 7.4?

I think the error messages has changed. I pushed a change to the test to expect a different error message <8.0.

Hard to test, I cannot replicate that setup locally easily.

@petermein petermein requested a review from derrabus January 6, 2025 14:52
@derrabus
Copy link
Member

derrabus commented Jan 6, 2025

Still failing.

Hard to test, I cannot replicate that setup locally easily.

What setup? PHP 7.4? What OS are you on?

@petermein petermein force-pushed the peter.mein/bugfix-mysqli-dealloc-prepared-statement branch 2 times, most recently from dab3355 to 29fe1e0 Compare January 7, 2025 08:45
@petermein
Copy link
Contributor Author

petermein commented Jan 7, 2025

Still failing.

Hard to test, I cannot replicate that setup locally easily.

What setup? PHP 7.4? What OS are you on?

I found a docker way to replicate it. With some docker magic.

My current attempt is with a deprecated PHPUnit function. Which will throw an E_DEPRECATION. @derrabus do you have any suggestion on how to make this prettier?

        if (PHP_VERSION_ID < 80000) {
            $this->expectError();
            $this->expectErrorMessage('mysqli_stmt::execute(): Couldn\'t fetch mysqli_stmt');
        } else {
            $this->expectException(Error::class);
            $this->expectExceptionMessage('mysqli_stmt object is already closed');
        }

@petermein
Copy link
Contributor Author

I would propose marking the test as skipped for 7.4, not ideal but I don't see a way to do properly.

petermein and others added 2 commits January 16, 2025 09:19
Long running processes might hit the `max_prepared_stmt_count` due not
deallocating the statement correctly.
@derrabus derrabus force-pushed the peter.mein/bugfix-mysqli-dealloc-prepared-statement branch from 29fe1e0 to 845ca16 Compare January 16, 2025 08:20
@derrabus
Copy link
Member

I've pushed a fix for your test.

@derrabus derrabus merged commit ec16c82 into doctrine:3.9.x Jan 16, 2025
64 of 65 checks passed
@derrabus
Copy link
Member

Thank you, @petermein!

derrabus added a commit that referenced this pull request Jan 16, 2025
* 3.9.x:
  bugfix: deallocate mysqli prepared statement (#6681)
derrabus added a commit to derrabus/dbal that referenced this pull request Jan 16, 2025
* 3.9.x:
  bugfix: deallocate mysqli prepared statement (doctrine#6681)
derrabus added a commit to derrabus/dbal that referenced this pull request Jan 16, 2025
* 3.9.x:
  bugfix: deallocate mysqli prepared statement (doctrine#6681)
derrabus added a commit that referenced this pull request Jan 16, 2025
* 4.2.x:
  bugfix: deallocate mysqli prepared statement (#6681)
derrabus added a commit that referenced this pull request Jan 16, 2025
* 4.3.x:
  bugfix: deallocate mysqli prepared statement (#6681)
@petermein
Copy link
Contributor Author

Thank you for the merge, I contemplated overriding the error reporting but I deemed it too rigorous 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants