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

Support result.free #692

Closed
wants to merge 1 commit into from

Conversation

@kamipo
Copy link
Contributor

commented Oct 18, 2015

Fixes #690.

When calling mysql_stmt_execute, we should not call mysql_stmt_close
before calling mysql_stmt_free_result.

Example:

require 'mysql2'

client = Mysql2::Client.new(
  host: 'localhost',
  username: 'root',
  database: 'mysql',
)

stmt = client.prepare('SELECT 1 FROM dual WHERE 0 = ?')
stmt.execute(1).tap { |result|
  # should call result.free before calling stmt.close
  result.free if result.respond_to?(:free)
}

stmt.close
# Segmentation fault
GC.start
Support result.free
Fixes #690.

When calling `mysql_stmt_execute`, we should not call `mysql_stmt_close`
before calling `mysql_stmt_free_result`.

@kamipo kamipo force-pushed the kamipo:support_result_free branch from 9ec9efa to 14986f9 Oct 18, 2015

@sodabrew sodabrew added this to the 0.4.2 milestone Oct 18, 2015

@sodabrew

This comment has been minimized.

Copy link
Collaborator

commented Nov 8, 2015

I don't want to put the user in a position that a failure to call result.free will crash. How about keeping track of the most recent Result in the Statement object, and if a new statement.execute or Statement object is GCd, then it'll free the last Result at that time?

@@ -48,7 +48,7 @@ static rb_encoding *binaryEncoding;
#define MYSQL2_MIN_TIME 62171150401ULL
#endif

#define GET_RESULT(obj) \
#define GET_RESULT(self) \

This comment has been minimized.

Copy link
@sodabrew

sodabrew Nov 8, 2015

Collaborator

Oh, nice catch on this.

@sodabrew

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2015

Closing in favor of #697 - but if Result#free turns out to be required for the ActiveRecord adapter, then we'll follow up on this.

@sodabrew sodabrew closed this Nov 22, 2015

@sodabrew sodabrew removed this from the 0.4.2 milestone Nov 22, 2015

@kamipo kamipo deleted the kamipo:support_result_free branch Nov 23, 2015

@sodabrew

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2016

I was wrong, I think we need this. Could you rebase the commits up to current master?

@sodabrew sodabrew added this to the 0.4.3 milestone Feb 9, 2016

@kamipo kamipo restored the kamipo:support_result_free branch Feb 14, 2016

@kamipo kamipo referenced this pull request Feb 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.