CDRIVER-2638 retry scan on some errs

kevinAlbs · kevinAlbs · commit 52004fb841d4 · 2018-05-17T16:13:45.000-04:00
diff --git a/src/mongoc/mongoc-client.c b/src/mongoc/mongoc-client.c
@@ -1556,7 +1556,7 @@ _mongoc_client_retryable_write_command_with_stream (
     * server does not support retryable writes, fall through and allow the
     * original error to be reported. */
    if (!ret && is_retryable && (error->domain == MONGOC_ERROR_STREAM ||
-                                mongoc_cluster_is_not_master_error (error))) {
+      mongoc_cluster_is_not_master_or_recovering_error (error))) {
       bson_error_t ignored_error;
 
       /* each write command may be retried at most once */
diff --git a/src/mongoc/mongoc-cluster-private.h b/src/mongoc/mongoc-cluster-private.h
@@ -72,7 +72,7 @@ typedef struct _mongoc_cluster_t {
 } mongoc_cluster_t;
 
 bool
-mongoc_cluster_is_not_master_error (const bson_error_t *error);
+mongoc_cluster_is_not_master_or_recovering_error (const bson_error_t *error);
 
 void
 mongoc_cluster_init (mongoc_cluster_t *cluster,
diff --git a/src/mongoc/mongoc-cluster.c b/src/mongoc/mongoc-cluster.c
@@ -406,11 +406,18 @@ mongoc_cluster_run_command_opquery (mongoc_cluster_t *cluster,
 }
 
 
+bool
+mongoc_cluster_is_not_master_or_recovering_error (const bson_error_t *error)
+{
+   return strstr (error->message, "not master") ||
+          strstr (error->message, "node is recovering");
+}
+
+
 bool
 mongoc_cluster_is_not_master_error (const bson_error_t *error)
 {
-   return !strncmp (error->message, "not master", 10) ||
-          !strncmp (error->message, "node is recovering", 18);
+   return strstr (error->message, "not master") != NULL;
 }
 
 
@@ -419,13 +426,32 @@ handle_not_master_error (mongoc_cluster_t *cluster,
                          uint32_t server_id,
                          const bson_error_t *error)
 {
-   if (mongoc_cluster_is_not_master_error (error)) {
+   mongoc_topology_t *topology = cluster->client->topology;
+
+   if (mongoc_cluster_is_not_master_or_recovering_error (error)) {
       /* Server Discovery and Monitoring Spec: "When the client sees a 'not
        * master' or 'node is recovering' error it MUST replace the server's
        * description with a default ServerDescription of type Unknown."
        */
-      mongoc_topology_invalidate_server (
-         cluster->client->topology, server_id, error);
+      mongoc_topology_invalidate_server (topology, server_id, error);
+      if (topology->single_threaded) {
+         /* SDAM Spec: "For single-threaded clients, in the case of a 'not
+          * master' error, the client MUST check the server immediately... For a
+          * 'node is recovering' error, single-threaded clients MUST NOT check
+          * the server, as an immediate server check is unlikely to find a
+          * usable server."
+          * Instead of an immediate check, mark the topology as stale so the
+          * next command scans all servers (to find the new primary). */
+         if (mongoc_cluster_is_not_master_error (error)) {
+            cluster->client->topology->stale = true;
+         }
+      } else {
+         /* SDAM Spec: "Multi-threaded and asynchronous clients MUST request an
+          * immediate check of the server."
+          * Instead of requesting a check of the one server, request a scan
+          * to all servers (to find the new primary). */
+         _mongoc_topology_request_scan (topology);
+      }
    }
 }
 
diff --git a/src/mongoc/mongoc-collection.c b/src/mongoc/mongoc-collection.c
@@ -3048,7 +3048,7 @@ mongoc_collection_find_and_modify_with_opts (
     * server does not support retryable writes, fall through and allow the
     * original error to be reported. */
    if (!ret && is_retryable && (error->domain == MONGOC_ERROR_STREAM ||
-                                mongoc_cluster_is_not_master_error (error))) {
+      mongoc_cluster_is_not_master_or_recovering_error (error))) {
       bson_error_t ignored_error;
 
       /* each write command may be retried at most once */
diff --git a/src/mongoc/mongoc-topology-private.h b/src/mongoc/mongoc-topology-private.h
@@ -151,5 +151,6 @@ void
 _mongoc_topology_do_blocking_scan (mongoc_topology_t *topology,
                                    bson_error_t *error);
 bson_t* _mongoc_topology_get_ismaster (mongoc_topology_t* topology);
-
+void
+_mongoc_topology_request_scan (mongoc_topology_t *topology);
 #endif
diff --git a/src/mongoc/mongoc-topology-scanner-private.h b/src/mongoc/mongoc-topology-scanner-private.h
@@ -88,7 +88,6 @@ typedef struct mongoc_topology_scanner {
    mongoc_topology_scanner_setup_err_cb_t setup_err_cb;
    mongoc_topology_scanner_cb_t cb;
    void *cb_data;
-   bool in_progress;
    const mongoc_uri_t *uri;
    mongoc_async_cmd_setup_t setup;
    mongoc_stream_initiator_t initiator;
@@ -193,6 +192,10 @@ mongoc_topology_scanner_set_ssl_opts (mongoc_topology_scanner_t *ts,
                                       mongoc_ssl_opt_t *opts);
 #endif
 
+bool
+mongoc_topology_scanner_node_in_cooldown (mongoc_topology_scanner_node_t *node,
+                                          int64_t when);
+
 /* for testing. */
 mongoc_stream_t *
 _mongoc_topology_scanner_tcp_initiate (mongoc_async_cmd_t *acmd);
diff --git a/src/mongoc/mongoc-topology-scanner.c b/src/mongoc/mongoc-topology-scanner.c
@@ -814,7 +814,7 @@ mongoc_topology_scanner_node_setup (mongoc_topology_scanner_node_t *node,
  *
  *--------------------------------------------------------------------------
  */
-static bool
+bool
 mongoc_topology_scanner_node_in_cooldown (mongoc_topology_scanner_node_t *node,
                                           int64_t when)
 {
@@ -860,7 +860,9 @@ mongoc_topology_scanner_in_cooldown (mongoc_topology_scanner_t *ts,
  *
  *      Initializes the scanner and begins a full topology check. This
  *      should be called once before calling mongoc_topology_scanner_work()
- *      repeatedly to complete the scan.
+ *      to complete the scan.
+ *
+ *      The topology mutex must be held by the caller.
  *
  *      If "obey_cooldown" is true, this is a single-threaded blocking scan
  *      that must obey the Server Discovery And Monitoring Spec's cooldownMS:
@@ -887,10 +889,6 @@ mongoc_topology_scanner_start (mongoc_topology_scanner_t *ts,
 
    BSON_ASSERT (ts);
 
-   if (ts->in_progress) {
-      return;
-   }
-
    _delete_retired_nodes (ts);
 
    now = bson_get_monotonic_time ();
diff --git a/src/mongoc/mongoc-topology.c b/src/mongoc/mongoc-topology.c
@@ -33,9 +33,6 @@
 static void
 _mongoc_topology_background_thread_stop (mongoc_topology_t *topology);
 
-static void
-_mongoc_topology_request_scan (mongoc_topology_t *topology);
-
 static bool
 _mongoc_topology_reconcile_add_nodes (mongoc_server_description_t *sd,
                                       mongoc_topology_t *topology)
@@ -888,7 +885,7 @@ _mongoc_topology_host_by_id (mongoc_topology_t *topology,
  *--------------------------------------------------------------------------
  */
 
-static void
+void
 _mongoc_topology_request_scan (mongoc_topology_t *topology)
 {
    topology->scan_requested = true;
@@ -1456,8 +1453,10 @@ _mongoc_topology_end_sessions_cmd (mongoc_topology_t *topology, bson_t *cmd)
 
 /* Locks topology->mutex and retrieves (possibly constructing) the handshake
  * on the topology scanner. */
-bson_t* _mongoc_topology_get_ismaster (mongoc_topology_t* topology) {
-   bson_t* cmd;
+bson_t *
+_mongoc_topology_get_ismaster (mongoc_topology_t *topology)
+{
+   bson_t *cmd;
    mongoc_mutex_lock (&topology->mutex);
    cmd = _mongoc_topology_scanner_get_ismaster (topology->scanner);
    mongoc_mutex_unlock (&topology->mutex);
diff --git a/src/mongoc/mongoc-write-command.c b/src/mongoc/mongoc-write-command.c
@@ -593,7 +593,7 @@ _mongoc_write_opmsg (mongoc_write_command_t *command,
           * and allow the original error to be reported. */
          if (!ret && is_retryable &&
              (error->domain == MONGOC_ERROR_STREAM ||
-              mongoc_cluster_is_not_master_error (error))) {
+                mongoc_cluster_is_not_master_or_recovering_error (error))) {
             bson_error_t ignored_error;
 
             /* each write command may be retried at most once */
diff --git a/tests/TestSuite.h b/tests/TestSuite.h
@@ -575,6 +575,7 @@ test_error (const char *format, ...) BSON_GNUC_PRINTF (1, 2);
                      BSON_FUNC);                                       \
             abort ();                                                  \
          }                                                             \
+         _mongoc_usleep (10 * 1000);                                   \
       }                                                                \
    } while (0)
 
diff --git a/tests/mock_server/mock-server.c b/tests/mock_server/mock-server.c
@@ -1936,3 +1936,50 @@ mock_server_set_bind_opts (mock_server_t *server, mock_server_bind_opts_t *opts)
 {
    server->bind_opts = *opts;
 }
+
+void
+rs_response_to_ismaster (mock_server_t *server, bool primary, int has_tags, ...)
+{
+   va_list ap;
+   bson_string_t *hosts;
+   bool first;
+   mock_server_t *host;
+   char *ismaster_response;
+
+   hosts = bson_string_new ("");
+
+   va_start (ap, has_tags);
+
+   first = true;
+   while ((host = va_arg (ap, mock_server_t *))) {
+      if (first) {
+         first = false;
+      } else {
+         bson_string_append (hosts, ",");
+      }
+
+      bson_string_append_printf (
+         hosts, "'%s'", mock_server_get_host_and_port (host));
+   }
+
+   va_end (ap);
+
+   ismaster_response = bson_strdup_printf ("{'ok': 1, "
+                                           " 'setName': 'rs',"
+                                           " 'ismaster': %s,"
+                                           " 'secondary': %s,"
+                                           " 'tags': {%s},"
+                                           " 'minWireVersion': 3,"
+                                           " 'maxWireVersion': 6,"
+                                           " 'hosts': [%s]"
+                                           "}",
+                                           primary ? "true" : "false",
+                                           primary ? "false" : "true",
+                                           has_tags ? "'key': 'value'" : "",
+                                           hosts->str);
+
+   mock_server_auto_ismaster (server, "%s", ismaster_response);
+
+   bson_free (ismaster_response);
+   bson_string_free (hosts, true);
+}
diff --git a/tests/mock_server/mock-server.h b/tests/mock_server/mock-server.h
@@ -209,4 +209,13 @@ mock_server_reply_multi (request_t *request,
 void
 mock_server_destroy (mock_server_t *server);
 
+void
+rs_response_to_ismaster (mock_server_t *server,
+                         bool primary,
+                         int has_tags,
+                         ...);
+
+#define RS_RESPONSE_TO_ISMASTER(server, primary, has_tags, ...) \
+   rs_response_to_ismaster (server, primary, has_tags, __VA_ARGS__, NULL)
+
 #endif /* MOCK_SERVER_H */
diff --git a/tests/test-mongoc-cluster.c b/tests/test-mongoc-cluster.c
@@ -937,13 +937,20 @@ typedef struct {
 } test_error_msg_t;
 
 
-test_error_msg_t errors[] = {{"not master", true},
-                             {"not master or secondary", true},
-                             {"node is recovering", true},
-                             {"foo", false},
-                             {0}};
-
-/* a "not master" or "node is recovering" error marks server Unknown */
+test_error_msg_t errors[] = {
+   {"not master", true},
+   {"not master or secondary", true},
+   {"node is recovering", true},
+   {"not master and slaveOk=false", true},
+   {"replicatedToNum called but not master anymore", true},
+   {"??? node is recovering ???", true},
+   {"??? not master ???", true},
+   {"foo", false},
+   {0}};
+
+/* a "not master" or "node is recovering" error marks server Unknown.
+   "not master" and "node is recovering" need only be substrings of the error
+   message. */
 static void
 _test_not_master (bool pooled,
                   bool use_op_msg,
diff --git a/tests/test-mongoc-topology-reconcile.c b/tests/test-mongoc-topology-reconcile.c
@@ -64,58 +64,6 @@ has_server_description (mongoc_topology_t *topology, const char *host_and_port)
 }
 
 
-void
-rs_response_to_ismaster (mock_server_t *server, bool primary, int has_tags, ...)
-{
-   va_list ap;
-   bson_string_t *hosts;
-   bool first;
-   mock_server_t *host;
-   char *ismaster_response;
-
-   hosts = bson_string_new ("");
-
-   va_start (ap, has_tags);
-
-   first = true;
-   while ((host = va_arg (ap, mock_server_t *))) {
-      if (first) {
-         first = false;
-      } else {
-         bson_string_append (hosts, ",");
-      }
-
-      bson_string_append_printf (
-         hosts, "'%s'", mock_server_get_host_and_port (host));
-   }
-
-   va_end (ap);
-
-   ismaster_response = bson_strdup_printf ("{'ok': 1, "
-                                           " 'setName': 'rs',"
-                                           " 'ismaster': %s,"
-                                           " 'secondary': %s,"
-                                           " 'tags': {%s},"
-                                           " 'minWireVersion': 2,"
-                                           " 'maxWireVersion': 5,"
-                                           " 'hosts': [%s]"
-                                           "}",
-                                           primary ? "true" : "false",
-                                           primary ? "false" : "true",
-                                           has_tags ? "'key': 'value'" : "",
-                                           hosts->str);
-
-   mock_server_auto_ismaster (server, ismaster_response);
-
-   bson_free (ismaster_response);
-   bson_string_free (hosts, true);
-}
-
-
-#define RS_RESPONSE_TO_ISMASTER(server, primary, has_tags, ...) \
-   rs_response_to_ismaster (server, primary, has_tags, __VA_ARGS__, NULL)
-
-
 bool
 selects_server (mongoc_client_t *client,
                 mongoc_read_prefs_t *read_prefs,
diff --git a/tests/test-mongoc-topology.c b/tests/test-mongoc-topology.c
