Save affected_rows on the wrapper when reading results #1383
Conversation
This file contains 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
Between fetching the result and accessing the affected_rows property, GC
might have been triggered and might have freed some Mysql2::Statement
objects. This calls mysql_stmt_close which resets the connection
affected_rows to -1, which in turn is treated as an error when calling
mysql_affected_rows.
```
client.query("SELECT 1")
client.affected_rows # raises Mysql2::Error
```
byroot
added a commit
to byroot/rails
that referenced
this pull request
Dec 2, 2024
Ref: brianmario/mysql2#1383 The proper fix can only be upstream, however we can reduce the likeliness of hitting the bug by only calling `#affected_rows` when strictly necessary, meaning when we performed a query that didn't return a result set.
byroot
added a commit
to byroot/rails
that referenced
this pull request
Dec 2, 2024
Ref: brianmario/mysql2#1383 The proper fix can only be upstream, however we can reduce the likeliness of hitting the bug by only calling `#affected_rows` when strictly necessary, meaning when we performed a query that didn't return a result set. Co-Authored-By: Étienne Barrié <etienne.barrie@gmail.com>
byroot
added a commit
to byroot/rails
that referenced
this pull request
Dec 2, 2024
Ref: brianmario/mysql2#1383 If we eagerly close uncached statements, we drastically reduce the risk of one of them being GCed between `#query` and `#affected_rows`. The bug is still there thouhg, and can happen when `#execute` is used.
byroot
added a commit
to byroot/rails
that referenced
this pull request
Dec 2, 2024
Ref: brianmario/mysql2#1383 The proper fix can only be upstream, however we can reduce the likeliness of hitting the bug by only calling `#affected_rows` when strictly necessary, meaning when we performed a query that didn't return a result set. Co-Authored-By: Étienne Barrié <etienne.barrie@gmail.com>
byroot
added a commit
to byroot/rails
that referenced
this pull request
Dec 2, 2024
Ref: brianmario/mysql2#1383 If we eagerly close uncached statements, we drastically reduce the risk of one of them being GCed between `#query` and `#affected_rows`. The bug is still there thouhg, and can happen when `#execute` is used.
byroot
added a commit
to byroot/rails
that referenced
this pull request
Dec 2, 2024
Ref: brianmario/mysql2#1383 The proper fix can only be upstream, however we can reduce the likeliness of hitting the bug by only calling `#affected_rows` when strictly necessary, meaning when we performed a query that didn't return a result set. Co-Authored-By: Étienne Barrié <etienne.barrie@gmail.com>
byroot
added a commit
to byroot/rails
that referenced
this pull request
Dec 2, 2024
Ref: brianmario/mysql2#1383 If we eagerly close uncached statements, we drastically reduce the risk of one of them being GCed between `#query` and `#affected_rows`. The bug is still there thouhg, and can happen when `#execute` is used.
sodabrew
reviewed
Dec 2, 2024
| @@ -1155,12 +1157,12 @@ static VALUE rb_mysql_client_session_track(VALUE self, VALUE type) { | |||
| * if it was an UPDATE, DELETE, or INSERT. | |||
| */ | |||
| static VALUE rb_mysql_client_affected_rows(VALUE self) { | |||
| my_ulonglong retVal; | |||
| uint64_t retVal; | |||
There was a problem hiding this comment.
Ok, I see the type changed between MySQL 5.7 and 8.0:
https://dev.mysql.com/doc/c-api/8.0/en/mysql-affected-rows.html
uint64_t
mysql_affected_rows(MYSQL *mysql)
https://dev.mysql.com/doc/c-api/5.7/en/mysql-affected-rows.html
my_ulonglong
mysql_affected_rows(MYSQL *mysql)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Between fetching the result and accessing the affected_rows property, GC might have been triggered and might have freed some Mysql2::Statement objects. This calls mysql_stmt_close which resets the connection affected_rows to -1, which in turn is treated as an error when calling mysql_affected_rows.
cc @matthewd @zzak (This is what is causing the flaky test on Rails).
@tenderlove You think you could merge and release this? This bugs make prepare statements fundamentally unsafe to use....