Close several unclosed clients in tests

[dylanahsmith]

Problem

I noticed some flaky test failures that seem like they were related to unclosed connections, like this test failure in https://travis-ci.org/brianmario/mysql2/jobs/235381348

  1) Mysql2::Client should terminate connections when calling close
     Failure/Error:
       expect {
         Mysql2::Client.new(DatabaseCredentials['root']).close
       }.to_not change {
         @client.query("SHOW STATUS LIKE 'Aborted_%'").to_a +
           @client.query("SHOW STATUS LIKE 'Threads_connected'").to_a
       }
     
       expected result not to have changed, but did change from [{"Variable_name"=>"Aborted_clients", "Value"=>"0"}, {"Variable_name"=>"Aborted_connects", "Value"=>"0"}, {"Variable_name"=>"Threads_connected", "Value"=>"6"}] to [{"Variable_name"=>"Aborted_clients", "Value"=>"0"}, {"Variable_name"=>"Aborted_connects", "Value"=>"0"}, {"Variable_name"=>"Threads_connected", "Value"=>"5"}]
     # ./spec/mysql2/client_spec.rb:174:in `block (2 levels) in <top (required)>'

Notice that the test failed because an extra connection was closed. Also, there were 6 Threads_connected at the start of the test, when there should only be one for @client.

Solution

I went through the tests looking for unclosed clients and added a close call for the ones I found.

[dylanahsmith]

That same test I mentioned in the PR description is failing, but this time with Threads_connected increasing by 1 after opening a connection and closing it. Seems like this test is flaky on master.

  1) Mysql2::Client should terminate connections when calling close
     Failure/Error:
       expect {
         Mysql2::Client.new(DatabaseCredentials['root']).close
       }.to_not change {
         @client.query("SHOW STATUS LIKE 'Aborted_%'").to_a +
           @client.query("SHOW STATUS LIKE 'Threads_connected'").to_a
       }
     
       expected result not to have changed, but did change from [{"Variable_name"=>"Aborted_clients", "Value"=>"0"}, {"Variable_name"=>"Aborted_connects", "Value"=>"0"}, {"Variable_name"=>"Threads_connected", "Value"=>"1"}] to [{"Variable_name"=>"Aborted_clients", "Value"=>"0"}, {"Variable_name"=>"Aborted_connects", "Value"=>"0"}, {"Variable_name"=>"Threads_connected", "Value"=>"2"}]
     # ./spec/mysql2/client_spec.rb:178:in `block (2 levels) in <top (required)>'

It looks like the mysql server might not have synchronously closed the connection when receiving a the QUIT message. Otherwise, there must be something else connecting to the database in the middle of the test.

Either way, the flaky test failure isn't caused by this PR.

[dylanahsmith]

I can reproduce the flaky test failure locally using the following changes to make it happen more reliably

diff --git a/spec/mysql2/client_spec.rb b/spec/mysql2/client_spec.rb
index dab4036..3390b60 100644
--- a/spec/mysql2/client_spec.rb
+++ b/spec/mysql2/client_spec.rb
@@ -171,12 +171,13 @@ RSpec.describe Mysql2::Client do
   end

   it "should terminate connections when calling close" do
-    expect {
-      Mysql2::Client.new(DatabaseCredentials['root']).close
-    }.to_not change {
-      @client.query("SHOW STATUS LIKE 'Aborted_%'").to_a +
+    10_000.times do |i|
+      expect {
+        Mysql2::Client.new(DatabaseCredentials['root']).close
+      }.to_not change {
         @client.query("SHOW STATUS LIKE 'Threads_connected'").to_a
-    }
+      }
+    end
   end

   it "should not leave dangling connections after garbage collection" do

[dylanahsmith]

mysql_close is implemented using simple_command(mysql,COM_QUIT,(uchar*) 0,0,1) where that last argument is skip_check which cli_advanced_command checks with

  if (!skip_check)
    result= ((mysql->packet_length=cli_safe_read(mysql)) == packet_error ?
	     1 : 0);

so it doesn't wait for the response to the quit command, which is what makes this race condition feasible. I'll open another PR to address this, since this PR is dealing with a different reason why that test can fail.

[jeremy]

Nice. This feels like a lot of litter in the specs themselves, though. Perhaps there's an abstraction we could introduce to both establish a connection and add it to a list of connections in use, then let an after { @clients.each &:close } clean up.

before { @clients = [] }
after { @clients.each &:close }
def connect(*args)
  Mysql2::Client.new(*args).tap { |c| @clients << c }
end

or a block form to close right away:

def connect(*args)
  client = Mysql2::Client.new(*args)
  if block_given?
    begin
      yield client
    ensure
      client.close
    end
  else
    @clients << client
  end
  client
end

[jeremy]

Perhaps clearer to have these tests simply not set @client. Let them be responsible for creating and closing this separate client instance.

[jeremy]

Feels odd to close the client before instantiating it. Should this be in an after { … }?

[dylanahsmith]

spec/spec_helper.rb already created a client. This one creates another one with a change in its configuration.

[jeremy]

Right, just feels odd to start a test by cleaning up after setup.

[jeremy]

(Note that after { … } is equivalent to after(:each) { … })

[dylanahsmith]

I didn't know that. All the other test files use after(:each), so should I keep it as after(:each) for consistency?

[dylanahsmith]

Introduced an abstraction as you suggested for creating clients, which avoid the problems with spec files wanting to override @client with one with a different configuration. I decided to call it new_client since it is a wrapper around Mysql2::Client.new

[sodabrew]

Well this is awesome!

[sodabrew]

The test at line 265 is much worse now - could you take another look to be sure that you're exercising that a parent processes's connection is not garbage collected and closed by a child process that inherits the connection?

  1) Mysql2::Client#automatic_close should not close connections when running in a child process
     Failure/Error: expect { client.query('SELECT 1') }.to_not raise_exception
     
       expected no Exception, got #<Mysql2::Error: MySQL server has gone away> with backtrace:
         # ./lib/mysql2/client.rb:120:in `_query'
         # ./lib/mysql2/client.rb:120:in `block in query'
         # ./lib/mysql2/client.rb:119:in `handle_interrupt'
         # ./lib/mysql2/client.rb:119:in `query'
         # ./spec/mysql2/client_spec.rb:265:in `block (4 levels) in <top (required)>'
         # ./spec/mysql2/client_spec.rb:265:in `block (3 levels) in <top (required)>'
     # ./spec/mysql2/client_spec.rb:265:in `block (3 levels) in <top (required)>'

[sodabrew]

Please rebase following merge of #846 and #853. Thanks!