Use vio_is_connected to check connected state #1022

csfrancis · 2019-02-01T19:34:35Z

Problem

The CONNECTED define which is used by the closed? method, does not detect if the server has remotely closed the connection. In this case, should the remote side terminate the connection, the call to closed? will incorrectly return true. As a result, it's possible for subsequent query to raise an exception once the client actually detects that the connection has been closed.

Solution

Use the vio_is_connected function to detect if the connection has been closed. The implementation will perform a non-blocking read on the socket to determine if the connection has been closed.

This function has been available since at least MySQL 5.6.x (from what I've Googled), so should be reliable. The symbol is globally visible as part of libmysqlclient shared library, but is not declared in the libmysqlclient headers (which is why I had to declare it inline). It does not seem as though the API has changed between 5.6 -> 8.0, so I feel safe using it here.

sodabrew

Thanks! Bunch of comments in the review. Really appreciate the detailed notes in the PR message.

sodabrew · 2019-02-01T20:29:50Z

ext/mysql2/client.c

@@ -26,7 +26,8 @@ static ID intern_brackets, intern_merge, intern_merge_bang, intern_new_with_args
  }

 #if defined(HAVE_MYSQL_NET_VIO) || defined(HAVE_ST_NET_VIO)
-  #define CONNECTED(wrapper) (wrapper->client->net.vio != NULL && wrapper->client->net.fd != -1)
+  my_bool vio_is_connected(Vio *vio);
+  #define CONNECTED(wrapper) (wrapper->client->net.vio != NULL && wrapper->client->net.fd != -1 && vio_is_connected(wrapper->client->net.vio))
 #elif defined(HAVE_MYSQL_NET_PVIO) || defined(HAVE_ST_NET_PVIO)
  #define CONNECTED(wrapper) (wrapper->client->net.pvio != NULL && wrapper->client->net.fd != -1)


Please add the vio_is_connected call here as well, or a reason why it's not there (such as, and I don't know the truth of this, if net.pvio is older versions that don't have this function in the library at all).

sodabrew · 2019-02-01T20:30:51Z

spec/mysql2/client_spec.rb

@@ -573,7 +586,7 @@ def run_gc
      end
      expect do
        @client.query("SELECT SLEEP(1)")
-      end.to raise_error(Mysql2::Error, /Lost connection to MySQL server/)
+      end.to raise_error(Mysql2::Error, 'MySQL client is not connected')


Why the different error text?

I think it's because we're validating the connection before executing the query (and it's now failing) and it raises with this error text.

sodabrew · 2019-02-01T20:33:09Z

ext/mysql2/client.c

@@ -26,7 +26,8 @@ static ID intern_brackets, intern_merge, intern_merge_bang, intern_new_with_args
  }

 #if defined(HAVE_MYSQL_NET_VIO) || defined(HAVE_ST_NET_VIO)
-  #define CONNECTED(wrapper) (wrapper->client->net.vio != NULL && wrapper->client->net.fd != -1)
+  my_bool vio_is_connected(Vio *vio);


Please move this to a header file and add an autoconf test that checks if the header is available in the given client library (there are more client libraries than just libmysqlclient).

👍 Yeah, there's some symbol resolution issues I'm seeing in CI. I'm going to add some autoconf-style checks to clean this up.

csfrancis · 2019-02-01T20:57:46Z

Thanks for the quick review @sodabrew! Looking at CI, there are a bunch of compatibility issues between versions which I'm going to investigate. I'll ping once I'm ready for another look.

sodabrew · 2019-02-01T22:38:22Z

Very welcome! There's an older PR that was also going to make a vio call, and I'd been trying to avoid that, but you're making me rethink. See #807 and #962.

csfrancis · 2019-02-02T01:52:42Z

Can you take another look @sodabrew?

I'm not super happy with this, but I can't really come up with a better way. The problem is that vio_is_connected is not always exported (it depends on the libmysqlclient package being installed). What I don't like about this is that the behaviour of closed? is going to depend on whether or not the function is available. I added a private _has_vio_is_connected? method that can be used to determine if the function is available, but it still feels a bit gross to me.

Another option would be to duplicate the st_vio structure from violite.h in the gem, as the is_connected function available as a member on the structure. I don't like this as much because it's very brittle -- if the underlying struct changes there's no easy way to tell.

A couple of the CI test environments are failing, but the failures don't seem related to these changes.

kirs · 2019-02-08T11:27:44Z

Thinking about _has_vio_is_connected?, should it be a class level method so you can do Mysql2::Client._has_vio_is_connected? before the connection is established? Alternatively could it be the part of Mysql2::Client.info hash?

csfrancis · 2019-02-10T20:03:57Z

@kirs I think removing _has_vio_is_connected? as a private instance method and making it available from the class is a good idea. I prefer the approach of adding it to the Mysql2::Client.info hash, rather than polluting the class with a separate method. I really hate having to expose this implementation detail to the client, but I think it's better to be explicit and make the information available, rather than try to have the client infer it in some other way.

csfrancis · 2019-02-12T14:42:51Z

@sodabrew Would you be able to take another look?

csfrancis added 2 commits February 1, 2019 15:25

Use vio_is_connected to check connected state

ba6bb52

Update rubocop

f52cb3b

csfrancis force-pushed the use-vio-is-connected branch from 71c2b75 to f52cb3b Compare February 1, 2019 20:26

sodabrew requested changes Feb 1, 2019

View reviewed changes

csfrancis force-pushed the use-vio-is-connected branch from 944c15c to c3bd301 Compare February 2, 2019 01:11

Check for vio_is_connected export

914f56b

csfrancis force-pushed the use-vio-is-connected branch from c3bd301 to 914f56b Compare February 2, 2019 01:51

csfrancis added 2 commits February 10, 2019 14:42

Remove fd check and rename macro

53ce628

Move _has_vio_is_connected? to info hash

37fc49d

Add fd check back to CONNECTED macro

b355917

sirupsen mentioned this pull request Mar 7, 2019

Asserting for DB failovers Shopify/toxiproxy-ruby#35

Closed

csfrancis mentioned this pull request Mar 12, 2019

Do not require an open connection for MySQL string escaping csfrancis/mysql2#1

Open

kirs mentioned this pull request Nov 3, 2023

Add connection check to Trilogy trilogy-libraries/trilogy#134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use vio_is_connected to check connected state #1022

Use vio_is_connected to check connected state #1022

csfrancis commented Feb 1, 2019

sodabrew left a comment

sodabrew Feb 1, 2019

sodabrew Feb 1, 2019

csfrancis Feb 1, 2019

sodabrew Feb 1, 2019

csfrancis Feb 1, 2019

csfrancis commented Feb 1, 2019

sodabrew commented Feb 1, 2019 •

edited

Loading

csfrancis commented Feb 2, 2019

kirs commented Feb 8, 2019

csfrancis commented Feb 10, 2019

csfrancis commented Feb 12, 2019

Use vio_is_connected to check connected state #1022

Are you sure you want to change the base?

Use vio_is_connected to check connected state #1022

Conversation

csfrancis commented Feb 1, 2019

Problem

Solution

sodabrew left a comment

Choose a reason for hiding this comment

sodabrew Feb 1, 2019

Choose a reason for hiding this comment

sodabrew Feb 1, 2019

Choose a reason for hiding this comment

csfrancis Feb 1, 2019

Choose a reason for hiding this comment

sodabrew Feb 1, 2019

Choose a reason for hiding this comment

csfrancis Feb 1, 2019

Choose a reason for hiding this comment

csfrancis commented Feb 1, 2019

sodabrew commented Feb 1, 2019 • edited Loading

csfrancis commented Feb 2, 2019

kirs commented Feb 8, 2019

csfrancis commented Feb 10, 2019

csfrancis commented Feb 12, 2019

sodabrew commented Feb 1, 2019 •

edited

Loading