From 64d7ff80819c652b24789fd08ff69c7db046b3da Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Wed, 16 Sep 2015 06:08:27 +0000 Subject: [PATCH 1/3] Allow the garbage collector to close connections --- ext/mysql2/client.c | 45 ++++++++++++++++++++++++++---- ext/mysql2/client.h | 1 + lib/mysql2/client.rb | 4 +-- spec/mysql2/client_spec.rb | 56 ++++++++++++++++++++++++++++++++++++++ spec/spec_helper.rb | 1 + 5 files changed, 99 insertions(+), 8 deletions(-) diff --git a/ext/mysql2/client.c b/ext/mysql2/client.c index 6d60647a6..44b9025fc 100644 --- a/ext/mysql2/client.c +++ b/ext/mysql2/client.c @@ -233,7 +233,7 @@ void decr_mysql2_client(mysql_client_wrapper *wrapper) if (wrapper->refcount == 0) { #ifndef _WIN32 - if (wrapper->connected) { + if (wrapper->connected && !wrapper->automatic_close) { /* The client is being garbage collected while connected. Prevent * mysql_close() from sending a mysql-QUIT or from calling shutdown() on * the socket by invalidating it. invalidate_fd() will drop this @@ -260,6 +260,11 @@ static VALUE allocate(VALUE klass) { obj = Data_Make_Struct(klass, mysql_client_wrapper, rb_mysql_client_mark, rb_mysql_client_free, wrapper); wrapper->encoding = Qnil; MARK_CONN_INACTIVE(self); +#ifndef _WIN32 + wrapper->automatic_close = 0; +#else + wrapper->automatic_close = 1; +#endif wrapper->server_version = 0; wrapper->reconnect_enabled = 0; wrapper->connect_timeout = 0; @@ -382,12 +387,9 @@ static VALUE rb_connect(VALUE self, VALUE user, VALUE pass, VALUE host, VALUE po /* * Terminate the connection; call this when the connection is no longer needed. - * The garbage collector can close the connection, but doing so emits an - * "Aborted connection" error on the server and increments the Aborted_clients - * status variable. + * To have the garbage collector close the connection, enable +automatic_close+. * - * @see http://dev.mysql.com/doc/en/communication-errors.html - * @return [void] + * @return [nil] */ static VALUE rb_mysql_client_close(VALUE self) { GET_CLIENT(self); @@ -1085,6 +1087,35 @@ static VALUE rb_mysql_client_encoding(VALUE self) { } #endif +/* call-seq: + * client.automatic_close? + * + * @return [Boolean] + */ +static VALUE get_automatic_close(VALUE self) { + GET_CLIENT(self); + return wrapper->automatic_close ? Qtrue : Qfalse; +} + +/* call-seq: + * client.automatic_close = true + * + * Set this to +true+ to let the garbage collector close this connection. + */ +static VALUE set_automatic_close(VALUE self, VALUE value) { + GET_CLIENT(self); + if (RTEST(value)) { + wrapper->automatic_close = 1; + } else { +#ifndef _WIN32 + wrapper->automatic_close = 0; +#else + rb_raise(cMysql2Error, "Connections are always closed by garbage collector on Windows"); +#endif + } + return value; +} + /* call-seq: * client.reconnect = true * @@ -1268,6 +1299,8 @@ void init_mysql2_client() { rb_define_method(cMysql2Client, "more_results?", rb_mysql_client_more_results, 0); rb_define_method(cMysql2Client, "next_result", rb_mysql_client_next_result, 0); rb_define_method(cMysql2Client, "store_result", rb_mysql_client_store_result, 0); + rb_define_method(cMysql2Client, "automatic_close?", get_automatic_close, 0); + rb_define_method(cMysql2Client, "automatic_close=", set_automatic_close, 1); rb_define_method(cMysql2Client, "reconnect=", set_reconnect, 1); rb_define_method(cMysql2Client, "warning_count", rb_mysql_client_warning_count, 0); rb_define_method(cMysql2Client, "query_info_string", rb_mysql_info, 0); diff --git a/ext/mysql2/client.h b/ext/mysql2/client.h index 3e29d6076..672eef974 100644 --- a/ext/mysql2/client.h +++ b/ext/mysql2/client.h @@ -43,6 +43,7 @@ typedef struct { int reconnect_enabled; unsigned int connect_timeout; int active; + int automatic_close; int connected; int initialized; int refcount; diff --git a/lib/mysql2/client.rb b/lib/mysql2/client.rb index bc2445ea9..ebd75670c 100644 --- a/lib/mysql2/client.rb +++ b/lib/mysql2/client.rb @@ -30,10 +30,10 @@ def initialize(opts = {}) opts[:connect_timeout] = 120 unless opts.key?(:connect_timeout) # TODO: stricter validation rather than silent massaging - [:reconnect, :connect_timeout, :local_infile, :read_timeout, :write_timeout, :default_file, :default_group, :secure_auth, :init_command].each do |key| + [:reconnect, :connect_timeout, :local_infile, :read_timeout, :write_timeout, :default_file, :default_group, :secure_auth, :init_command, :automatic_close].each do |key| next unless opts.key?(key) case key - when :reconnect, :local_infile, :secure_auth + when :reconnect, :local_infile, :secure_auth, :automatic_close send(:"#{key}=", !!opts[key]) # rubocop:disable Style/DoubleNegation when :connect_timeout, :read_timeout, :write_timeout send(:"#{key}=", opts[key].to_i) diff --git a/spec/mysql2/client_spec.rb b/spec/mysql2/client_spec.rb index a1af17e9f..045d1725b 100644 --- a/spec/mysql2/client_spec.rb +++ b/spec/mysql2/client_spec.rb @@ -210,6 +210,62 @@ def run_gc expect { client.query('SELECT 1') }.to_not raise_exception end + context "#automatic_close" do + if RUBY_PLATFORM =~ /mingw|mswin/ + it "is enabled by default" do + client = Mysql2::Client.new(DatabaseCredentials['root']) + expect(client.automatic_close?).to be(true) + end + + it "cannot be disabled" do + expect { + Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => false)) + }.to raise_error(Mysql2::Error) + + client = Mysql2::Client.new(DatabaseCredentials['root']) + + expect { client.automatic_close = false }.to raise_error(Mysql2::Error) + expect { client.automatic_close = true }.to_not raise_error + end + else + it "is disabled by default" do + client = Mysql2::Client.new(DatabaseCredentials['root']) + expect(client.automatic_close?).to be(false) + end + + it "can be configured" do + client = Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => true)) + expect(client.automatic_close?).to be(true) + end + + it "can be assigned" do + client = Mysql2::Client.new(DatabaseCredentials['root']) + client.automatic_close = true + expect(client.automatic_close?).to be(true) + + client.automatic_close = false + expect(client.automatic_close?).to be(false) + + client.automatic_close = 9 + expect(client.automatic_close?).to be(true) + + client.automatic_close = nil + expect(client.automatic_close?).to be(false) + end + end + + it "should terminate connections during garbage collection" do + run_gc + expect { + Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => true)).query('SELECT 1') + run_gc + }.to_not change { + @client.query("SHOW STATUS LIKE 'Aborted_%'").to_a + + @client.query("SHOW STATUS LIKE 'Threads_connected'").to_a + } + end + end + it "should be able to connect to database with numeric-only name" do creds = DatabaseCredentials['numericuser'] @client.query "CREATE DATABASE IF NOT EXISTS `#{creds['database']}`" diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 73c45819e..30d84adbf 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -90,5 +90,6 @@ def with_internal_encoding(encoding) "test", "test", 'val1', 'val1,val2' ) ] + client.close end end From 100435c1ea96656bbfe9b79dcba137aa758e547c Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Wed, 16 Sep 2015 06:55:04 +0000 Subject: [PATCH 2/3] Close connections during garbage collection by default --- ext/mysql2/client.c | 18 ++++--- spec/mysql2/client_spec.rb | 105 ++++++++++++++++--------------------- spec/spec_helper.rb | 1 - 3 files changed, 55 insertions(+), 69 deletions(-) diff --git a/ext/mysql2/client.c b/ext/mysql2/client.c index 44b9025fc..c88cad78b 100644 --- a/ext/mysql2/client.c +++ b/ext/mysql2/client.c @@ -260,11 +260,7 @@ static VALUE allocate(VALUE klass) { obj = Data_Make_Struct(klass, mysql_client_wrapper, rb_mysql_client_mark, rb_mysql_client_free, wrapper); wrapper->encoding = Qnil; MARK_CONN_INACTIVE(self); -#ifndef _WIN32 - wrapper->automatic_close = 0; -#else wrapper->automatic_close = 1; -#endif wrapper->server_version = 0; wrapper->reconnect_enabled = 0; wrapper->connect_timeout = 0; @@ -386,8 +382,10 @@ static VALUE rb_connect(VALUE self, VALUE user, VALUE pass, VALUE host, VALUE po } /* - * Terminate the connection; call this when the connection is no longer needed. - * To have the garbage collector close the connection, enable +automatic_close+. + * Immediately disconnect from the server; normally the garbage collector + * will disconnect automatically when a connection is no longer needed. + * Explicitly closing this will free up server resources sooner than waiting + * for the garbage collector. * * @return [nil] */ @@ -1098,9 +1096,13 @@ static VALUE get_automatic_close(VALUE self) { } /* call-seq: - * client.automatic_close = true + * client.automatic_close = false + * + * Set this to +false+ to leave the connection open after it is garbage + * collected. To avoid "Aborted connection" errors on the server, explicitly + * call +close+ when the connection is no longer needed. * - * Set this to +true+ to let the garbage collector close this connection. + * @see http://dev.mysql.com/doc/en/communication-errors.html */ static VALUE set_automatic_close(VALUE self, VALUE value) { GET_CLIENT(self); diff --git a/spec/mysql2/client_spec.rb b/spec/mysql2/client_spec.rb index 045d1725b..39137d3ad 100644 --- a/spec/mysql2/client_spec.rb +++ b/spec/mysql2/client_spec.rb @@ -166,57 +166,36 @@ def run_gc expect { Mysql2::Client.new(DatabaseCredentials['root']).close }.to_not change { - @client.query("SHOW STATUS LIKE 'Aborted_clients'").first['Value'].to_i + @client.query("SHOW STATUS LIKE 'Aborted_%'").to_a + + @client.query("SHOW STATUS LIKE 'Threads_connected'").to_a } end it "should not leave dangling connections after garbage collection" do run_gc + expect { + expect { + 10.times do + Mysql2::Client.new(DatabaseCredentials['root']).query('SELECT 1') + end + }.to change { + @client.query("SHOW STATUS LIKE 'Threads_connected'").first['Value'].to_i + }.by(10) - client = Mysql2::Client.new(DatabaseCredentials['root']) - before_count = client.query("SHOW STATUS LIKE 'Threads_connected'").first['Value'].to_i - - 10.times do - Mysql2::Client.new(DatabaseCredentials['root']).query('SELECT 1') - end - after_count = client.query("SHOW STATUS LIKE 'Threads_connected'").first['Value'].to_i - expect(after_count).to eq(before_count + 10) - - run_gc - final_count = client.query("SHOW STATUS LIKE 'Threads_connected'").first['Value'].to_i - expect(final_count).to eq(before_count) - end - - it "should not close connections when running in a child process" do - pending("fork is not available on this platform") unless Process.respond_to?(:fork) - - run_gc - client = Mysql2::Client.new(DatabaseCredentials['root']) - - # this empty `fork` call fixes this tests on RBX; without it, the next - # `fork` call hangs forever. WTF? - fork {} - - fork do - client.query('SELECT 1') - client = nil run_gc - end - - Process.wait - - # this will throw an error if the underlying socket was shutdown by the - # child's GC - expect { client.query('SELECT 1') }.to_not raise_exception + }.to_not change { + @client.query("SHOW STATUS LIKE 'Aborted_%'").to_a + + @client.query("SHOW STATUS LIKE 'Threads_connected'").to_a + } end context "#automatic_close" do - if RUBY_PLATFORM =~ /mingw|mswin/ - it "is enabled by default" do - client = Mysql2::Client.new(DatabaseCredentials['root']) - expect(client.automatic_close?).to be(true) - end + it "is enabled by default" do + client = Mysql2::Client.new(DatabaseCredentials['root']) + expect(client.automatic_close?).to be(true) + end + if RUBY_PLATFORM =~ /mingw|mswin/ it "cannot be disabled" do expect { Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => false)) @@ -228,41 +207,47 @@ def run_gc expect { client.automatic_close = true }.to_not raise_error end else - it "is disabled by default" do - client = Mysql2::Client.new(DatabaseCredentials['root']) - expect(client.automatic_close?).to be(false) - end - it "can be configured" do - client = Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => true)) - expect(client.automatic_close?).to be(true) + client = Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => false)) + expect(client.automatic_close?).to be(false) end it "can be assigned" do client = Mysql2::Client.new(DatabaseCredentials['root']) - client.automatic_close = true - expect(client.automatic_close?).to be(true) - client.automatic_close = false expect(client.automatic_close?).to be(false) - client.automatic_close = 9 + client.automatic_close = true expect(client.automatic_close?).to be(true) client.automatic_close = nil expect(client.automatic_close?).to be(false) + + client.automatic_close = 9 + expect(client.automatic_close?).to be(true) end - end - it "should terminate connections during garbage collection" do - run_gc - expect { - Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => true)).query('SELECT 1') + it "should not close connections when running in a child process" do run_gc - }.to_not change { - @client.query("SHOW STATUS LIKE 'Aborted_%'").to_a + - @client.query("SHOW STATUS LIKE 'Threads_connected'").to_a - } + client = Mysql2::Client.new(DatabaseCredentials['root']) + client.automatic_close = false + + # this empty `fork` call fixes this tests on RBX; without it, the next + # `fork` call hangs forever. WTF? + fork {} + + fork do + client.query('SELECT 1') + client = nil + run_gc + end + + Process.wait + + # this will throw an error if the underlying socket was shutdown by the + # child's GC + expect { client.query('SELECT 1') }.to_not raise_exception + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 30d84adbf..73c45819e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -90,6 +90,5 @@ def with_internal_encoding(encoding) "test", "test", 'val1', 'val1,val2' ) ] - client.close end end From 890345e0e4d0829f3656ae9c79fc6dc021c30571 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Sat, 23 Jan 2016 13:26:33 +0000 Subject: [PATCH 3/3] Emit warning warning when changing automatic_close on Windows --- ext/mysql2/client.c | 2 +- spec/mysql2/client_spec.rb | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/ext/mysql2/client.c b/ext/mysql2/client.c index c88cad78b..ae1eab582 100644 --- a/ext/mysql2/client.c +++ b/ext/mysql2/client.c @@ -1112,7 +1112,7 @@ static VALUE set_automatic_close(VALUE self, VALUE value) { #ifndef _WIN32 wrapper->automatic_close = 0; #else - rb_raise(cMysql2Error, "Connections are always closed by garbage collector on Windows"); + rb_warn("Connections are always closed by garbage collector on Windows"); #endif } return value; diff --git a/spec/mysql2/client_spec.rb b/spec/mysql2/client_spec.rb index 39137d3ad..9faad5e10 100644 --- a/spec/mysql2/client_spec.rb +++ b/spec/mysql2/client_spec.rb @@ -1,5 +1,6 @@ # encoding: UTF-8 require 'spec_helper' +require 'stringio' RSpec.describe Mysql2::Client do context "using defaults file" do @@ -197,14 +198,22 @@ def run_gc if RUBY_PLATFORM =~ /mingw|mswin/ it "cannot be disabled" do - expect { + stderr, $stderr = $stderr, StringIO.new + + begin Mysql2::Client.new(DatabaseCredentials['root'].merge(:automatic_close => false)) - }.to raise_error(Mysql2::Error) + expect($stderr.string).to include('always closed by garbage collector') + $stderr.reopen - client = Mysql2::Client.new(DatabaseCredentials['root']) + client = Mysql2::Client.new(DatabaseCredentials['root']) + client.automatic_close = false + expect($stderr.string).to include('always closed by garbage collector') + $stderr.reopen - expect { client.automatic_close = false }.to raise_error(Mysql2::Error) - expect { client.automatic_close = true }.to_not raise_error + expect { client.automatic_close = true }.to_not change { $stderr.string } + ensure + $stderr = stderr + end end else it "can be configured" do