Skip to content

CDRIVER-3929 Include apiVersion in handshake commands #753

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 8 commits into from
Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-client-pool.c
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ struct _mongoc_client_pool_t {
int32_t error_api_version;
bool error_api_set;
mongoc_server_api_t *api;
bool client_initialized;
Copy link
Contributor

Choose a reason for hiding this comment

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

to clarify, this is meant to be set when the first client created by this pool is initialized?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

};


Expand Down Expand Up @@ -240,6 +241,8 @@ _initialize_new_client (mongoc_client_pool_t *pool, mongoc_client_t *client)
pool->topology->scanner->initiator,
pool->topology->scanner->initiator_context);

pool->client_initialized = true;
client->is_pooled = true;
client->error_api_version = pool->error_api_version;
_mongoc_client_set_apm_callbacks_private (
client, &pool->apm_callbacks, pool->apm_context);
Expand Down Expand Up @@ -528,6 +531,15 @@ mongoc_client_pool_set_server_api (mongoc_client_pool_t *pool,
return false;
}

if (pool->client_initialized) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use pool->size > 0 here instead and lose the new flag? Unless you specified a max pool size of 0 (which I don't think would allow the pool to create any clients at all) then pool->size should always be > 0 if a client has been created.

Copy link
Member Author

@alcaeus alcaeus Mar 24, 2021

Choose a reason for hiding this comment

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

On second thought, that would've worked. I had this in previously but refactored this since I thought the logic would allow changes when pool->size == 0. Reading the logic in mongoc_client_pool_push again, this case can't happen as old clients are only dropped from the pool if pool->min_pool_size is trueish, in which case you also can't have the last client dropped. I can change it back to the previous check with pool->size, but I feel like the flag is more explicit.

bson_set_error (error,
MONGOC_ERROR_POOL,
MONGOC_ERROR_POOL_API_TOO_LATE,
"Cannot set server api after a client has been created");
return false;
}

pool->api = mongoc_server_api_copy (api);
_mongoc_topology_scanner_set_server_api (pool->topology->scanner, api);
return true;
}
1 change: 1 addition & 0 deletions src/libmongoc/src/mongoc/mongoc-client-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ struct _mongoc_client_t {
mongoc_uri_t *uri;
mongoc_cluster_t cluster;
bool in_exhaust;
bool is_pooled;

mongoc_stream_initiator_t initiator;
void *initiator_data;
Expand Down
9 changes: 9 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-client.c
Original file line number Diff line number Diff line change
Expand Up @@ -3105,6 +3105,14 @@ mongoc_client_set_server_api (mongoc_client_t *client,
BSON_ASSERT_PARAM (client);
BSON_ASSERT_PARAM (api);

if (client->is_pooled) {
bson_set_error (error,
MONGOC_ERROR_CLIENT,
MONGOC_ERROR_CLIENT_API_FROM_POOL,
"Cannot set server api on a client checked out from a pool");
return false;
}

if (client->api) {
bson_set_error (error,
MONGOC_ERROR_CLIENT,
Expand All @@ -3114,5 +3122,6 @@ mongoc_client_set_server_api (mongoc_client_t *client,
}

client->api = mongoc_server_api_copy (api);
_mongoc_topology_scanner_set_server_api (client->topology->scanner, api);
return true;
}
4 changes: 4 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-cmd-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ _is_retryable_read (const mongoc_cmd_parts_t *parts,
void
_mongoc_cmd_append_payload_as_array (const mongoc_cmd_t *cmd, bson_t *out);

void
_mongoc_cmd_append_server_api (bson_t *command_body,
const mongoc_server_api_t *api);

BSON_END_DECLS


Expand Down
65 changes: 44 additions & 21 deletions src/libmongoc/src/mongoc/mongoc-cmd.c
Original file line number Diff line number Diff line change
Expand Up @@ -796,8 +796,8 @@ _txn_in_progress (mongoc_cmd_parts_t *parts)
}

return (_mongoc_client_session_txn_in_progress (cs)
/* commitTransaction and abortTransaction count as in progress, too. */
|| parts->assembled.is_txn_finish);
/* commitTransaction and abortTransaction count as in progress, too. */
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated whitespace change, but the tabs really stood out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh that's from my text editor, I'm not sure why clang-format didn't fix it for me. Sorry!

|| parts->assembled.is_txn_finish);
}


Expand Down Expand Up @@ -827,14 +827,12 @@ mongoc_cmd_parts_assemble (mongoc_cmd_parts_t *parts,
bson_error_t *error)
{
mongoc_server_description_type_t server_type;
mongoc_server_api_t *api;
mongoc_client_session_t *cs;
const bson_t *cluster_time = NULL;
mongoc_read_prefs_t *prefs = NULL;
const char *cmd_name;
bool is_get_more;
const mongoc_read_prefs_t *prefs_ptr;
const char *string_version;
bool ret = false;

ENTRY;
Expand Down Expand Up @@ -994,23 +992,8 @@ mongoc_cmd_parts_assemble (mongoc_cmd_parts_t *parts,
sending a getmore, or if we are in a transaction. */
if (parts->client->api) {
if (!is_get_more && !_txn_in_progress (parts)) {
api = parts->client->api;
string_version = mongoc_server_api_version_to_string (api->version);

bson_append_utf8 (
&parts->assembled_body, "apiVersion", -1, string_version, -1);

if (api->strict.is_set) {
bson_append_bool (
&parts->assembled_body, "apiStrict", -1, api->strict.value);
}

if (api->deprecation_errors.is_set) {
bson_append_bool (&parts->assembled_body,
"apiDeprecationErrors",
-1,
api->deprecation_errors.value);
}
_mongoc_cmd_append_server_api (&parts->assembled_body,
parts->client->api);
}
}

Expand Down Expand Up @@ -1155,3 +1138,43 @@ _mongoc_cmd_append_payload_as_array (const mongoc_cmd_t *cmd, bson_t *out)

bson_append_array_end (out, &bson);
}

/*--------------------------------------------------------------------------
*
* _mongoc_cmd_append_server_api --
* Append versioned API fields to a mongoc_cmd_t
*
* Arguments:
* cmd The mongoc_cmd_t, which will have versioned API fields added
* api A mongoc_server_api_t holding server API information
*
* Pre-conditions:
* - @api is initialized.
* - @command_body is initialised
*
*--------------------------------------------------------------------------
*/
void
_mongoc_cmd_append_server_api (bson_t *command_body,
const mongoc_server_api_t *api)
{
const char *string_version;

BSON_ASSERT (command_body);
BSON_ASSERT (api);

string_version = mongoc_server_api_version_to_string (api->version);

bson_append_utf8 (command_body, "apiVersion", -1, string_version, -1);

if (api->strict.is_set) {
bson_append_bool (command_body, "apiStrict", -1, api->strict.value);
}

if (api->deprecation_errors.is_set) {
bson_append_bool (command_body,
"apiDeprecationErrors",
-1,
api->deprecation_errors.value);
}
}
4 changes: 3 additions & 1 deletion src/libmongoc/src/mongoc/mongoc-error.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ typedef enum {

/* An error related to server version api */
MONGOC_ERROR_CLIENT_API_ALREADY_SET,
MONGOC_ERROR_POOL_API_ALREADY_SET
MONGOC_ERROR_CLIENT_API_FROM_POOL,
MONGOC_ERROR_POOL_API_ALREADY_SET,
MONGOC_ERROR_POOL_API_TOO_LATE
} mongoc_error_code_t;

MONGOC_EXPORT (bool)
Expand Down
6 changes: 6 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-topology-scanner-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ typedef struct mongoc_topology_scanner {
bool negotiate_sasl_supported_mechs;
bool bypass_cooldown;
bool speculative_authentication;

mongoc_server_api_t *api;
} mongoc_topology_scanner_t;

mongoc_topology_scanner_t *
Expand Down Expand Up @@ -223,6 +225,10 @@ bool
mongoc_topology_scanner_node_in_cooldown (mongoc_topology_scanner_node_t *node,
int64_t when);

void
_mongoc_topology_scanner_set_server_api (mongoc_topology_scanner_t *ts,
const mongoc_server_api_t *api);

/* for testing. */
mongoc_stream_t *
_mongoc_topology_scanner_tcp_initiate (mongoc_async_cmd_t *acmd);
Expand Down
48 changes: 41 additions & 7 deletions src/libmongoc/src/mongoc/mongoc-topology-scanner.c
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,32 @@ _jumpstart_other_acmds (mongoc_topology_scanner_node_t *node,
mongoc_async_cmd_t *acmd);

static void
_add_ismaster (bson_t *cmd)
_add_ismaster (bson_t *cmd, const mongoc_server_api_t *api)
{
BSON_APPEND_INT32 (cmd, "isMaster", 1);

if (api) {
_mongoc_cmd_append_server_api (cmd, api);
}
}

static void
_init_ismaster (mongoc_topology_scanner_t *ts)
{
bson_init (&ts->ismaster_cmd);
bson_init (&ts->ismaster_cmd_with_handshake);
bson_init (&ts->cluster_time);

_add_ismaster (&ts->ismaster_cmd, ts->api);
}

static void
_reset_ismaster (mongoc_topology_scanner_t *ts)
{
bson_reinit (&ts->ismaster_cmd);
bson_reinit (&ts->ismaster_cmd_with_handshake);

_add_ismaster (&ts->ismaster_cmd, ts->api);
}

const char *
Expand Down Expand Up @@ -226,7 +249,7 @@ _build_ismaster_with_handshake (mongoc_topology_scanner_t *ts)
int count = 0;
char buf[16];

_add_ismaster (doc);
_add_ismaster (doc, ts->api);

BSON_APPEND_DOCUMENT_BEGIN (doc, HANDSHAKE_FIELD, &subdoc);
res = _mongoc_handshake_build_doc_with_application (&subdoc, ts->appname);
Expand Down Expand Up @@ -346,21 +369,19 @@ mongoc_topology_scanner_new (

ts->async = mongoc_async_new ();

bson_init (&ts->ismaster_cmd);
_add_ismaster (&ts->ismaster_cmd);
bson_init (&ts->ismaster_cmd_with_handshake);
bson_init (&ts->cluster_time);

ts->setup_err_cb = setup_err_cb;
ts->cb = cb;
ts->cb_data = data;
ts->uri = uri;
ts->appname = NULL;
ts->api = NULL;
ts->handshake_ok_to_send = false;
ts->connect_timeout_msec = connect_timeout_msec;
/* may be overridden for testing. */
ts->dns_cache_timeout_ms = DNS_CACHE_TIMEOUT_MS;

_init_ismaster (ts);

return ts;
}

Expand Down Expand Up @@ -398,6 +419,7 @@ mongoc_topology_scanner_destroy (mongoc_topology_scanner_t *ts)
bson_destroy (&ts->ismaster_cmd);
bson_destroy (&ts->ismaster_cmd_with_handshake);
bson_destroy (&ts->cluster_time);
mongoc_server_api_destroy (ts->api);

/* This field can be set by a mongoc_client */
bson_free ((char *) ts->appname);
Expand Down Expand Up @@ -1324,3 +1346,15 @@ _jumpstart_other_acmds (mongoc_topology_scanner_node_t *node,
}
}
}

void
_mongoc_topology_scanner_set_server_api (mongoc_topology_scanner_t *ts,
const mongoc_server_api_t *api)
{
BSON_ASSERT (ts);
BSON_ASSERT (api);

mongoc_server_api_destroy (ts->api);
ts->api = mongoc_server_api_copy (api);
_reset_ismaster (ts);
}
Loading