Skip to content

CDRIVER-5589 add option to prefer TCP for SRV lookup #1625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 4, 2024

Conversation

kevinAlbs
Copy link
Collaborator

@kevinAlbs kevinAlbs commented Jun 3, 2024

Summary

Add option to prefer TCP for SRV lookup.

Checks for the environment variable MONGOC_SRV_PREFER_TCP=ON.

Verified with this patch build.

Background & Motivation

See DRIVERS-2919 for a description of the problem this is intended to solve. DRIVERS-2919 proposes a solution for all drivers. This PR is intended as a libmongoc-only solution for the problem impacting HELP-59749.

Rejected alternatives

Adding an option setter

Initially considered was adding an option setter:

MONGOC_EXPORT (void)
mongoc_client_pool_set_srv_prefer_tcp (mongoc_client_pool_t *pool, bool value);

SRV lookup first occurs in mongoc_client_pool_new_with_error. An option setter like mongoc_client_pool_set_ssl_opts requires a client pool to already be constructed.

Adding a new constructor

Also considered was a new mongoc_client_pool_new_with_opts constructor and new options struct:

MONGOC_EXPORT (mongoc_client_pool_opts_t *)
mongoc_client_pool_opts_new (void);

MONGOC_EXPORT (void)
mongoc_client_pool_opts_destroy (mongoc_client_pool_opts_t *opts);

MONGOC_EXPORT (void)
mongoc_client_pool_opts_set_srv_prefer_tcp (mongoc_client_pool_opts_t *opts, bool value);

MONGOC_EXPORT (mongoc_client_pool_t *)
mongoc_client_pool_new_with_opts (const mongoc_uri_t *uri,
                                  mongoc_client_pool_opts_t *opts,
                                  bson_error_t *error) BSON_GNUC_WARN_UNUSED_RESULT;

However, this breaks established patterns for passing options to the client pool. I prefer to avoid a significant API addition.

Adding a URI option

Also considered was adding a URI option srvPreferTCP. This may result in a non-portable URI connection string. URIs are intended to be portable among drivers:

Connection Strings should be portable

Check for the environment variable MONGOC_SRV_PREFER_TCP=ON.

Option is deliberately undocumented. This option is not portable among drivers. This option does not apply to the implementation using `res_search`
@kevinAlbs kevinAlbs changed the title add option to prefer TCP for SRV lookup CDRIVER-5589 add option to prefer TCP for SRV lookup Jun 3, 2024
@kevinAlbs kevinAlbs marked this pull request as ready for review June 4, 2024 11:15
Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinAlbs kevinAlbs requested a review from eramongodb June 4, 2024 15:42
@@ -383,6 +383,14 @@ mongoc_topology_new (const mongoc_uri_t *uri, bool single_threaded)
#endif

topology = (mongoc_topology_t *) bson_malloc0 (sizeof *topology);
// Check if requested to use TCP for SRV lookup.
{
char *MONGOC_EXPERIMENTAL_SRV_PREFER_TCP = _mongoc_getenv ("MONGOC_EXPERIMENTAL_SRV_PREFER_TCP");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

char *MONGOC_EXPERIMENTAL_SRV_PREFER_TCP -> char *srv_prefer_tcp (not a macro).

@kevinAlbs kevinAlbs merged commit 43ce760 into mongodb:master Jun 4, 2024
8 of 9 checks passed
kevinAlbs added a commit that referenced this pull request Jun 4, 2024
Check for the environment variable `MONGOC_EXPERIMENTAL_SRV_PREFER_TCP`.

Option is documented as experimental and subject to change. This option is not portable among drivers. This option does not apply to the implementation using `res_search`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants