CDRIVER-3723 finalize URI during client/pool construction (#921)

Introduces new mongoc_client_new_from_uri_with_error and mongoc_client_pool_new_with_error methods. Also addresses topology leak in mongoc_client_new_from_uri (CDRIVER-4251).

* Remove redundant warnings about unused results

* Explain why we need not handle null result from _mongoc_client_new_from_topology

* Log URI errors in mongoc_client_new as warnings

* Clear logs between /Client/max_staleness assertions

* Set error instead of logging in mongoc_client_pool_new_with_error

* Remove test expecting a client with an invalid topology

* Revise assertions in /loadbalanced/client_uri_validation test

This no longer requires constructing a client and expecting an error through server selection.

* Use tracing macro for return statement

* Check MONGOC_ENABLE_SSL in mongoc_client_new_from_uri_with_error

Moving the check up from _mongoc_client_new_from_topology makes mongoc_client_new_from_uri_with_error consistent with mongoc_client_pool_new_with_error. Additionally, we add log message assertions to corresponding tests.

* Anticipate null pool or client in SRV error tests

* Remove obsolete tests for invalid LB topologies

These tests were previously introduced in CDRIVER-4184 but no longer apply now that we prohibit construction of clients and pools with an invalid topology.

* _mongoc_client_new_from_topology should require a valid topology

* Document precondition for _mongoc_client_new_from_topology

Co-authored-by: Ezra Chung <[email protected]>
Co-authored-by: Kevin Albertson <[email protected]>

Author: 3 people

src/libmongoc/doc/mongoc_client_new.rst

@@ -21,7 +21,7 @@ Parameters
 Returns
 -------
 
-A newly allocated :symbol:`mongoc_client_t` if the URI parsed successfully, otherwise ``NULL``.
+A newly allocated :symbol:`mongoc_client_t` that should be freed with :symbol:`mongoc_client_destroy()` when no longer in use. On error, ``NULL`` is returned and an error or warning will be logged.
 
 .. seealso::
 

src/libmongoc/doc/mongoc_client_new_from_uri.rst

@@ -22,5 +22,9 @@ Parameters
 Returns
 -------
 
-A newly allocated :symbol:`mongoc_client_t` that should be freed with :symbol:`mongoc_client_destroy()` when no longer in use.
+A newly allocated :symbol:`mongoc_client_t` that should be freed with :symbol:`mongoc_client_destroy()` when no longer in use. On error, ``NULL`` is returned and an error will be logged.
+
+.. seealso::
+
+  | :symbol:`mongoc_client_new_from_uri_with_error()`
 

src/libmongoc/doc/mongoc_client_new_from_uri_with_error.rst

@@ -0,0 +1,28 @@
+:man_page: mongoc_client_new_from_uri_with_error
+
+mongoc_client_new_from_uri_with_error()
+=======================================
+
+Synopsis
+--------
+
+.. code-block:: c
+
+  mongoc_client_t *
+  mongoc_client_new_from_uri_with_error (const mongoc_uri_t *uri,
+                                         bson_error_t *error)
+     BSON_GNUC_WARN_UNUSED_RESULT;
+
+Creates a new :symbol:`mongoc_client_t` using the :symbol:`mongoc_uri_t` provided.
+
+Parameters
+----------
+
+* ``uri``: A :symbol:`mongoc_uri_t`.
+* ``error``: An optional location for a :symbol:`bson_error_t <errors>` or ``NULL``.
+
+Returns
+-------
+
+A newly allocated :symbol:`mongoc_client_t` that should be freed with :symbol:`mongoc_client_destroy()` when no longer in use. On error, ``NULL`` is returned and ``error`` will be populated with the error description.
+

src/libmongoc/doc/mongoc_client_pool_new.rst

@@ -21,5 +21,9 @@ Parameters
 Returns
 -------
 
-A newly allocated :symbol:`mongoc_client_pool_t` that should be freed with :symbol:`mongoc_client_pool_destroy()` when no longer in use.
+A newly allocated :symbol:`mongoc_client_pool_t` that should be freed with :symbol:`mongoc_client_pool_destroy()` when no longer in use. On error, ``NULL`` is returned and an error may be logged.
+
+.. seealso::
+
+  | :symbol:`mongoc_client_pool_new_with_error()`
 

src/libmongoc/doc/mongoc_client_pool_new_with_error.rst

@@ -0,0 +1,28 @@
+:man_page: mongoc_client_pool_new_with_error
+
+mongoc_client_pool_new_with_error()
+===================================
+
+Synopsis
+--------
+
+.. code-block:: c
+
+  mongoc_client_pool_t *
+  mongoc_client_pool_new_with_error (const mongoc_uri_t *uri,
+                                     bson_error_t *error)
+     BSON_GNUC_WARN_UNUSED_RESULT;
+
+This function creates a new :symbol:`mongoc_client_pool_t` using the :symbol:`mongoc_uri_t` provided.
+
+Parameters
+----------
+
+* ``uri``: A :symbol:`mongoc_uri_t`.
+* ``error``: An optional location for a :symbol:`bson_error_t <errors>` or ``NULL``.
+
+Returns
+-------
+
+A newly allocated :symbol:`mongoc_client_pool_t` that should be freed with :symbol:`mongoc_client_pool_destroy()` when no longer in use. On error, ``NULL`` is returned and ``error`` will be populated with the error description.
+

src/libmongoc/doc/mongoc_client_pool_t.rst

@@ -35,6 +35,7 @@ Example
     mongoc_client_pool_max_size
     mongoc_client_pool_min_size
     mongoc_client_pool_new
+    mongoc_client_pool_new_with_error
     mongoc_client_pool_pop
     mongoc_client_pool_push
     mongoc_client_pool_set_apm_callbacks

src/libmongoc/doc/mongoc_client_t.rst

@@ -73,6 +73,7 @@ Example
     mongoc_client_get_write_concern
     mongoc_client_new
     mongoc_client_new_from_uri
+    mongoc_client_new_from_uri_with_error
     mongoc_client_read_command_with_opts
     mongoc_client_read_write_command_with_opts
     mongoc_client_reset

src/libmongoc/src/mongoc/mongoc-client-pool.c

@@ -98,6 +98,20 @@ _mongoc_client_pool_set_internal_tls_opts (
 
 mongoc_client_pool_t *
 mongoc_client_pool_new (const mongoc_uri_t *uri)
+{
+   mongoc_client_pool_t *pool;
+   bson_error_t error = {0};
+
+   if (!(pool = mongoc_client_pool_new_with_error (uri, &error))) {
+      MONGOC_ERROR ("%s", error.message);
+   }
+
+   return pool;
+}
+
+
+mongoc_client_pool_t *
+mongoc_client_pool_new_with_error (const mongoc_uri_t *uri, bson_error_t *error)
 {
    mongoc_topology_t *topology;
    mongoc_client_pool_t *pool;
@@ -112,12 +126,27 @@ mongoc_client_pool_new (const mongoc_uri_t *uri)
 
 #ifndef MONGOC_ENABLE_SSL
    if (mongoc_uri_get_tls (uri)) {
-      MONGOC_ERROR ("Can't create SSL client pool,"
-                    " SSL not enabled in this build.");
+      bson_set_error (error,
+                      MONGOC_ERROR_COMMAND,
+                      MONGOC_ERROR_COMMAND_INVALID_ARG,
+                      "Can't create SSL client pool, SSL not enabled in this "
+                      "build.");
       return NULL;
    }
 #endif
 
+   topology = mongoc_topology_new (uri, false);
+
+   if (!topology->valid) {
+      if (error) {
+         memcpy (error, &topology->scanner->error, sizeof (bson_error_t));
+      }
+
+      mongoc_topology_destroy (topology);
+
+      RETURN (NULL);
+   }
+
    pool = (mongoc_client_pool_t *) bson_malloc0 (sizeof *pool);
    bson_mutex_init (&pool->mutex);
    mongoc_cond_init (&pool->cond);
@@ -126,8 +155,6 @@ mongoc_client_pool_new (const mongoc_uri_t *uri)
    pool->min_pool_size = 0;
    pool->max_pool_size = 100;
    pool->size = 0;
-
-   topology = mongoc_topology_new (uri, false);
    pool->topology = topology;
    pool->error_api_version = MONGOC_ERROR_API_VERSION_LEGACY;
 
@@ -278,7 +305,8 @@ mongoc_client_pool_pop (mongoc_client_pool_t *pool)
 again:
    if (!(client = (mongoc_client_t *) _mongoc_queue_pop_head (&pool->queue))) {
       if (pool->size < pool->max_pool_size) {
-         client = _mongoc_client_new_from_uri (pool->topology);
+         client = _mongoc_client_new_from_topology (pool->topology);
+         BSON_ASSERT (client);
          _initialize_new_client (pool, client);
          pool->size++;
       } else {
@@ -321,7 +349,8 @@ mongoc_client_pool_try_pop (mongoc_client_pool_t *pool)
 
    if (!(client = (mongoc_client_t *) _mongoc_queue_pop_head (&pool->queue))) {
       if (pool->size < pool->max_pool_size) {
-         client = _mongoc_client_new_from_uri (pool->topology);
+         client = _mongoc_client_new_from_topology (pool->topology);
+         BSON_ASSERT (client);
          _initialize_new_client (pool, client);
          pool->size++;
       }

src/libmongoc/src/mongoc/mongoc-client-pool.h

@@ -39,6 +39,9 @@ typedef struct _mongoc_client_pool_t mongoc_client_pool_t;
 
 MONGOC_EXPORT (mongoc_client_pool_t *)
 mongoc_client_pool_new (const mongoc_uri_t *uri) BSON_GNUC_WARN_UNUSED_RESULT;
+MONGOC_EXPORT (mongoc_client_pool_t *)
+mongoc_client_pool_new_with_error (const mongoc_uri_t *uri, bson_error_t *error)
+   BSON_GNUC_WARN_UNUSED_RESULT;
 MONGOC_EXPORT (void)
 mongoc_client_pool_destroy (mongoc_client_pool_t *pool);
 MONGOC_EXPORT (mongoc_client_t *)

src/libmongoc/src/mongoc/mongoc-client-private.h

@@ -161,7 +161,7 @@ _mongoc_client_get_rr (const char *hostname,
                        bson_error_t *error);
 
 mongoc_client_t *
-_mongoc_client_new_from_uri (mongoc_topology_t *topology);
+_mongoc_client_new_from_topology (mongoc_topology_t *topology);
 
 bool
 _mongoc_client_set_apm_callbacks_private (mongoc_client_t *client,

src/libmongoc/src/mongoc/mongoc-client.c

@@ -982,48 +982,27 @@ _mongoc_client_recv (mongoc_client_t *client,
 }
 
 
-/*
- *--------------------------------------------------------------------------
- *
- * mongoc_client_new --
- *
- *       Create a new mongoc_client_t using the URI provided.
- *
- *       @uri should be a MongoDB URI string such as "mongodb://localhost/"
- *       More information on the format can be found at
- *       http://docs.mongodb.org/manual/reference/connection-string/
- *
- * Returns:
- *       A newly allocated mongoc_client_t or NULL if @uri_string is
- *       invalid.
- *
- * Side effects:
- *       None.
- *
- *--------------------------------------------------------------------------
- */
 mongoc_client_t *
 mongoc_client_new (const char *uri_string)
 {
-   mongoc_topology_t *topology;
    mongoc_client_t *client;
    mongoc_uri_t *uri;
-
+   bson_error_t error = {0};
 
    if (!uri_string) {
       uri_string = "mongodb://127.0.0.1/";
    }
 
-   if (!(uri = mongoc_uri_new (uri_string))) {
+   if (!(uri = mongoc_uri_new_with_error (uri_string, &error))) {
+      /* Log URI errors as a warning for consistency with mongoc_uri_new */
+      MONGOC_WARNING ("Error parsing URI: '%s'", error.message);
       return NULL;
    }
 
-   topology = mongoc_topology_new (uri, true);
-
-   client = _mongoc_client_new_from_uri (topology);
-   if (!client) {
-      mongoc_topology_destroy (topology);
+   if (!(client = mongoc_client_new_from_uri_with_error (uri, &error))) {
+      MONGOC_ERROR ("%s", error.message);
    }
+
    mongoc_uri_destroy (uri);
 
    return client;
@@ -1084,54 +1063,65 @@ mongoc_client_set_ssl_opts (mongoc_client_t *client,
 #endif
 
 
-/*
- *--------------------------------------------------------------------------
- *
- * mongoc_client_new_from_uri --
- *
- *       Create a new mongoc_client_t for a mongoc_uri_t.
- *
- * Returns:
- *       A newly allocated mongoc_client_t.
- *
- * Side effects:
- *       None.
- *
- *--------------------------------------------------------------------------
- */
-
 mongoc_client_t *
 mongoc_client_new_from_uri (const mongoc_uri_t *uri)
 {
+   mongoc_client_t *client;
+   bson_error_t error = {0};
+
+   if (!(client = mongoc_client_new_from_uri_with_error (uri, &error))) {
+      MONGOC_ERROR ("%s", error.message);
+   }
+
+   return client;
+}
+
+
+mongoc_client_t *
+mongoc_client_new_from_uri_with_error (const mongoc_uri_t *uri,
+                                       bson_error_t *error)
+{
+   mongoc_client_t *client;
    mongoc_topology_t *topology;
 
+
+   ENTRY;
+
+   BSON_ASSERT (uri);
+
+#ifndef MONGOC_ENABLE_SSL
+   if (mongoc_uri_get_tls (uri)) {
+      bson_set_error (
+         error,
+         MONGOC_ERROR_COMMAND,
+         MONGOC_ERROR_COMMAND_INVALID_ARG,
+         "Can't create SSL client, SSL not enabled in this build.");
+      RETURN (NULL);
+   }
+#endif
+
    topology = mongoc_topology_new (uri, true);
 
-   /* topology->uri may be different from uri: if this is a mongodb+srv:// URI
-    * then mongoc_topology_new has fetched SRV and TXT records and updated its
-    * uri from them.
-    */
-   return _mongoc_client_new_from_uri (topology);
+   if (!topology->valid) {
+      if (error) {
+         memcpy (error, &topology->scanner->error, sizeof (bson_error_t));
+      }
+
+      mongoc_topology_destroy (topology);
+
+      RETURN (NULL);
+   }
+
+   client = _mongoc_client_new_from_topology (topology);
+   BSON_ASSERT (client);
+
+   RETURN (client);
 }
 
-/*
- *--------------------------------------------------------------------------
- *
- * _mongoc_client_new_from_uri --
- *
- *       Create a new mongoc_client_t for a given topology object.
- *
- * Returns:
- *       A newly allocated mongoc_client_t.
- *
- * Side effects:
- *       None.
- *
- *--------------------------------------------------------------------------
- */
 
+/* precondition: topology is valid */
 mongoc_client_t *
-_mongoc_client_new_from_uri (mongoc_topology_t *topology)
+_mongoc_client_new_from_topology (mongoc_topology_t *topology)
 {
    mongoc_client_t *client;
    const mongoc_read_prefs_t *read_prefs;
@@ -1140,13 +1130,7 @@ _mongoc_client_new_from_uri (mongoc_topology_t *topology)
    const char *appname;
 
    BSON_ASSERT (topology);
-
-#ifndef MONGOC_ENABLE_SSL
-   if (mongoc_uri_get_tls (topology->uri)) {
-      MONGOC_ERROR ("Can't create SSL client, SSL not enabled in this build.");
-      return NULL;
-   }
-#endif
+   BSON_ASSERT (topology->valid);
 
    client = (mongoc_client_t *) bson_malloc0 (sizeof *client);
    client->uri = mongoc_uri_copy (topology->uri);

src/libmongoc/src/mongoc/mongoc-client.h

@@ -107,6 +107,9 @@ mongoc_client_new (const char *uri_string) BSON_GNUC_WARN_UNUSED_RESULT;
 MONGOC_EXPORT (mongoc_client_t *)
 mongoc_client_new_from_uri (const mongoc_uri_t *uri)
    BSON_GNUC_WARN_UNUSED_RESULT;
+MONGOC_EXPORT (mongoc_client_t *)
+mongoc_client_new_from_uri_with_error (
+   const mongoc_uri_t *uri, bson_error_t *error) BSON_GNUC_WARN_UNUSED_RESULT;
 MONGOC_EXPORT (const mongoc_uri_t *)
 mongoc_client_get_uri (const mongoc_client_t *client);
 MONGOC_EXPORT (void)