CDRIVER-2172 check idle socket with "ping"

Server Selection Spec update for single-threaded drivers.

If a server is selected that has an existing connection that has been idle for
socketCheckIntervalMS, the driver MUST check the connection with the "ping"
command. If the ping succeeds, use the selected connection. If not, set the
server's type to Unknown and update the Topology Description according to the
Server Discovery and Monitoring Spec, and attempt once more to select a server.

Author: ajdavis

src/mongoc/mongoc-client.c

@@ -2169,7 +2169,6 @@ mongoc_client_select_server (mongoc_client_t *client,
 {
    mongoc_ss_optype_t optype = for_writes ? MONGOC_SS_WRITE : MONGOC_SS_READ;
    mongoc_server_description_t *sd;
-   bson_error_t tmp_err;
 
    if (for_writes && prefs) {
       bson_set_error (error,
@@ -2188,7 +2187,7 @@ mongoc_client_select_server (mongoc_client_t *client,
       return NULL;
    }
 
-   if (mongoc_cluster_check_interval (&client->cluster, sd->id, &tmp_err)) {
+   if (mongoc_cluster_check_interval (&client->cluster, sd->id)) {
       /* check not required, or it succeeded */
       return sd;
    }
@@ -2200,7 +2199,6 @@ mongoc_client_select_server (mongoc_client_t *client,
       return sd;
    }
 
-   memcpy (error, &tmp_err, sizeof tmp_err);
    return NULL;
 }
 

src/mongoc/mongoc-cluster-private.h

@@ -102,8 +102,7 @@ _mongoc_cluster_buffer_iovec (mongoc_iovec_t *iov,
 
 bool
 mongoc_cluster_check_interval (mongoc_cluster_t *cluster,
-                               uint32_t server_id,
-                               bson_error_t *error);
+                               uint32_t server_id);
 
 bool
 mongoc_cluster_sendv_to_server (mongoc_cluster_t *cluster,

src/mongoc/mongoc-cluster.c

@@ -1979,6 +1979,16 @@ _mongoc_cluster_stream_for_optype (mongoc_cluster_t *cluster,
       RETURN (NULL);
    }
 
+   if (!mongoc_cluster_check_interval (cluster, server_id)) {
+      /* Server Selection Spec: try once more */
+      server_id = mongoc_topology_select_server_id (
+         topology, optype, read_prefs, error);
+
+      if (!server_id) {
+         RETURN (NULL);
+      }
+   }
+
    /* connect or reconnect to server if necessary */
    server_stream = _mongoc_cluster_stream_for_server (
       cluster, server_id, true /* reconnect_ok */, error);
@@ -2167,37 +2177,36 @@ mongoc_cluster_get_max_msg_size (mongoc_cluster_t *cluster)
  *
  * mongoc_cluster_check_interval --
  *
- *      Server Discovery And Monitoring Spec:
+ *      Server Selection Spec:
  *
- *      "Only for single-threaded clients. If a server is selected that has an
- *      existing connection that has been idle for socketCheckIntervalMS, the
- *      driver MUST check it, by using it for an ismaster call. Whether
- *      ismaster succeeds or fails, the driver MUST update its topology
- *      description with the outcome. Then, if the check failed, the driver
- *      MUST retry server selection once."
+ *      Only for single-threaded drivers.
+ *
+ *      If a server is selected that has an existing connection that has been
+ *      idle for socketCheckIntervalMS, the driver MUST check the connection
+ *      with the "ping" command. If the ping succeeds, use the selected
+ *      connection. If not, set the server's type to Unknown and update the
+ *      Topology Description according to the Server Discovery and Monitoring
+ *      Spec, and attempt once more to select a server.
  *
  * Returns:
  *      True if the check succeeded or no check was required, false if the
  *      check failed.
  *
  * Side effects:
- *      If a check is needed, updates the topology with its outcome.
+ *      If a check fails, closes stream and may set server type Unknown.
  *
  *--------------------------------------------------------------------------
  */
 
 bool
-mongoc_cluster_check_interval (mongoc_cluster_t *cluster,
-                               uint32_t server_id,
-                               bson_error_t *error)
+mongoc_cluster_check_interval (mongoc_cluster_t *cluster, uint32_t server_id)
 {
    mongoc_topology_t *topology;
    mongoc_topology_scanner_node_t *scanner_node;
    mongoc_stream_t *stream;
    int64_t now;
-   int64_t before_ismaster;
    bson_t command;
-   bson_t reply;
+   bson_error_t error;
    bool r = true;
 
    topology = cluster->client->topology;
@@ -2226,41 +2235,30 @@ mongoc_cluster_check_interval (mongoc_cluster_t *cluster,
    if (scanner_node->last_used + (1000 * CHECK_CLOSED_DURATION_MSEC) < now) {
       if (mongoc_stream_check_closed (stream)) {
          mongoc_cluster_disconnect_node (cluster, server_id);
-         bson_set_error (error,
-                         MONGOC_ERROR_STREAM,
-                         MONGOC_ERROR_STREAM_SOCKET,
-                         "Stream is closed");
          return false;
       }
    }
 
    if (scanner_node->last_used + (1000 * cluster->socketcheckintervalms) <
        now) {
       bson_init (&command);
-      BSON_APPEND_INT32 (&command, "ismaster", 1);
-
-      before_ismaster = now;
+      BSON_APPEND_INT32 (&command, "ping", 1);
       r = mongoc_cluster_run_command_private (cluster,
                                               stream,
                                               server_id,
                                               MONGOC_QUERY_SLAVE_OK,
                                               "admin",
                                               &command,
-                                              &reply,
-                                              error /* OUT */);
-
-      now = bson_get_monotonic_time ();
+                                              NULL,
+                                              &error /* OUT */);
 
       bson_destroy (&command);
 
-      mongoc_topology_description_handle_ismaster (&topology->description,
-                                                   server_id,
-                                                   &reply,
-                                                   (now - before_ismaster) /
-                                                      1000, /* RTT_MS */
-                                                   error /* IN */);
-
-      bson_destroy (&reply);
+      if (!r) {
+         mongoc_cluster_disconnect_node (cluster, server_id);
+         mongoc_topology_invalidate_server (
+            topology, server_id, &error /* IN */);
+      }
    }
 
    return r;

src/mongoc/mongoc-collection.c

@@ -353,30 +353,27 @@ mongoc_collection_aggregate (mongoc_collection_t *collection,       /* IN */
    if (server_id) {
       /* will set slaveok bit if server is not mongos */
       mongoc_cursor_set_hint (cursor, server_id);
-   } else {
-      server_id =
-         mongoc_topology_select_server_id (collection->client->topology,
-                                           MONGOC_SS_READ,
-                                           read_prefs,
+
+      /* server id isn't enough. ensure we're connected & know its wire version */
+      server_stream =
+         mongoc_cluster_stream_for_server (&collection->client->cluster,
+                                           cursor->server_id,
+                                           true /* reconnect ok */,
                                            &cursor->error);
 
-      if (!server_id) {
+      if (!server_stream) {
          GOTO (done);
       }
+   } else {
+      server_stream = mongoc_cluster_stream_for_reads (
+         &collection->client->cluster, read_prefs, &cursor->error);
 
-      /* don't use mongoc_cursor_set_hint, don't want special slaveok logic */
-      cursor->server_id = server_id;
-   }
-
-   /* server id isn't enough. ensure we're connected & know its wire version */
-   server_stream =
-      mongoc_cluster_stream_for_server (&collection->client->cluster,
-                                        cursor->server_id,
-                                        true /* reconnect ok */,
-                                        &cursor->error);
+      if (!server_stream) {
+         GOTO (done);
+      }
 
-   if (!server_stream) {
-      GOTO (done);
+      /* don't use mongoc_cursor_set_hint, don't want special slaveok logic */
+      cursor->server_id = server_stream->sd->id;
    }
 
    use_cursor = server_stream->sd->max_wire_version >= WIRE_VERSION_AGG_CURSOR;

src/mongoc/mongoc-server-description.c

@@ -57,6 +57,7 @@ mongoc_server_description_reset (mongoc_server_description_t *sd)
    /* set other fields to default or empty states. election_id is zeroed. */
    memset (
       &sd->set_name, 0, sizeof (*sd) - ((char *) &sd->set_name - (char *) sd));
+   memset (&sd->error, 0, sizeof sd->error);
    sd->set_name = NULL;
    sd->type = MONGOC_SERVER_UNKNOWN;
 

tests/test-mongoc-client.c

@@ -2229,8 +2229,7 @@ test_mongoc_client_select_server_error_pooled (void)
 
 
 /* CDRIVER-2172: in single mode, if the selected server has a socket that's been
- * idle for socketCheckIntervalMS, check it with an ismaster call. If that
- * fails, retry once.
+ * idle for socketCheckIntervalMS, check it with ping. If it fails, retry once.
  */
 static void
 _test_mongoc_client_select_server_retry (bool retry_succeeds)
@@ -2270,9 +2269,11 @@ _test_mongoc_client_select_server_retry (bool retry_succeeds)
    /* let socketCheckIntervalMS pass */
    _mongoc_usleep (100 * 1000);
 
-   /* second selection requires ismaster check, which fails */
+   /* second selection requires ping, which fails */
    future = future_client_select_server (client, true, NULL, &error);
-   request = mock_server_receives_ismaster (server);
+   request = mock_server_receives_command (
+      server, "admin", MONGOC_QUERY_SLAVE_OK, "{'ping': 1}");
+
    mock_server_hangs_up (request);
    request_destroy (request);
 
@@ -2311,6 +2312,94 @@ test_mongoc_client_select_server_retry_fail (void)
 }
 
 
+
+/* CDRIVER-2172: in single mode, if the selected server has a socket that's been
+ * idle for socketCheckIntervalMS, check it with ping. If it fails, retry once.
+ */
+static void
+_test_mongoc_client_fetch_stream_retry (bool retry_succeeds)
+{
+   char *ismaster;
+   mock_server_t *server;
+   mongoc_uri_t *uri;
+   mongoc_client_t *client;
+   bson_error_t error;
+   request_t *request;
+   future_t *future;
+
+   server = mock_server_new ();
+   mock_server_run (server);
+   ismaster = bson_strdup_printf ("{'ok': 1, 'ismaster': true}");
+   uri = mongoc_uri_copy (mock_server_get_uri (server));
+   mongoc_uri_set_option_as_int32 (uri, "socketCheckIntervalMS", 50);
+   client = mongoc_client_new_from_uri (uri);
+
+   /* first time succeeds */
+   future = future_client_command_simple (
+      client, "db", tmp_bson ("{'cmd': 1}"), NULL, NULL, &error);
+   request = mock_server_receives_ismaster (server);
+   mock_server_replies_simple (request, ismaster);
+   request_destroy (request);
+
+   request = mock_server_receives_command (
+      server, "db", MONGOC_QUERY_SLAVE_OK, "{'cmd': 1}");
+   mock_server_replies_simple (request, "{'ok': 1}");
+   request_destroy (request);
+
+   ASSERT_OR_PRINT (future_get_bool (future), error);
+   future_destroy (future);
+
+   /* let socketCheckIntervalMS pass */
+   _mongoc_usleep (100 * 1000);
+
+   /* second selection requires ping, which fails */
+   future = future_client_command_simple (
+      client, "db", tmp_bson ("{'cmd': 1}"), NULL, NULL, &error);
+
+   request = mock_server_receives_command (
+      server, "admin", MONGOC_QUERY_SLAVE_OK, "{'ping': 1}");
+
+   mock_server_hangs_up (request);
+   request_destroy (request);
+
+   /* mongoc_client_select_server retries once */
+   request = mock_server_receives_ismaster (server);
+   if (retry_succeeds) {
+      mock_server_replies_simple (request, ismaster);
+      request_destroy (request);
+
+      request = mock_server_receives_command (
+         server, "db", MONGOC_QUERY_SLAVE_OK, "{'cmd': 1}");
+
+      mock_server_replies_simple (request, "{'ok': 1}");
+      ASSERT_OR_PRINT (future_get_bool (future), error);
+   } else {
+      mock_server_hangs_up (request);
+      BSON_ASSERT (!future_get_bool (future));
+   }
+
+   future_destroy (future);
+   request_destroy (request);
+   mongoc_client_destroy (client);
+   mongoc_uri_destroy (uri);
+   bson_free (ismaster);
+   mock_server_destroy (server);
+}
+
+
+static void
+test_mongoc_client_fetch_stream_retry_succeed (void)
+{
+   _test_mongoc_client_fetch_stream_retry (true);
+}
+
+static void
+test_mongoc_client_fetch_stream_retry_fail (void)
+{
+   _test_mongoc_client_fetch_stream_retry (false);
+}
+
+
 #if defined(MONGOC_ENABLE_SSL_OPENSSL) || \
    defined(MONGOC_ENABLE_SSL_SECURE_TRANSPORT)
 static bool
@@ -3100,6 +3189,12 @@ test_client_install (TestSuite *suite)
    TestSuite_AddMockServerTest (suite,
                                 "/Client/select_server/retry/fail",
                                 test_mongoc_client_select_server_retry_fail);
+   TestSuite_AddMockServerTest (suite,
+                                "/Client/fetch_stream/retry/succeed",
+                                test_mongoc_client_fetch_stream_retry_succeed);
+   TestSuite_AddMockServerTest (suite,
+                                "/Client/fetch_stream/retry/fail",
+                                test_mongoc_client_fetch_stream_retry_fail);
    TestSuite_AddFull (suite,
                       "/Client/null_error_pointer/single",
                       test_null_error_pointer_single,