CDRIVER-3929 Include apiVersion in handshake commands #753

alcaeus · 2021-03-11T13:08:14Z

This fixes an oversight in the versioned API implementation where the handshake did not send API Version information to the server. This required exposing the API Version to the topology scanner as that's where the actual commands are handled. I've decided to allow changing the API Version multiple times as this part of libmongoc is very much internal.

During testing I discovered a couple of edge cases around setting API Version on a pooled client:

When no API Version has been set, a user can set it after clients have been popped. This causes an inconsistent state in the pool. Solution is to prevent changing the API Version when the pool size is greater than 0. I understand this can cause an inconsistency when the last client is destroyed where one could once again set the API Version. Let me know if we should also prevent that case.
When fetching a client from a pool that had no server API set, the api was not locked against modifications, which could once again lead to an inconsistent state in the pool. For clients fetched from a pool, the server API is mutable. This may apply to other properties of the client as well, so we may want to create a follow-up ticket for this.

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

alcaeus · 2021-03-11T13:09:13Z

src/libmongoc/src/mongoc/mongoc-cmd.c

@@ -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. */


Unrelated whitespace change, but the tabs really stood out.

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

samantharitter

I like the refactoring you did in cmd, and these changes seem like they are on the right track, just a couple of questions from me.

samantharitter · 2021-03-17T15:49:34Z

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

@@ -119,6 +119,7 @@ struct _mongoc_client_t {
   bool error_api_set;

   mongoc_server_api_t *api;
+   bool api_set;


Do we need this? Can we not rely on the api ptr being NULL if it hasn't been set?

I think this solves the issue of a client being created from a pool without an API set. The client's API pointer would be left NULL, but we wouldn't want to allow users to set the API pointer either.

That's exactly the reason. On second thought and after our conversation yesterday where you brought up CDRIVER-3537, I'll rename this to is_pooled and use it to track whether the client is part of a client pool. This allows me to print a more appropriate error message and also allow us to re-use the flag later when implementing other checks.

Sounds good!

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

samantharitter · 2021-03-17T15:53:06Z

src/libmongoc/src/mongoc/mongoc-topology-scanner.c

+_mongoc_topology_scanner_set_server_api (mongoc_topology_scanner_t *ts,
+                                         const mongoc_server_api_t *api)
+{
+   mongoc_server_api_t *prev_api;


Just curious why you didn't choose to call mongoc_server_api_destroy (ts->api); directly without shuffling it into a placeholder variable? mongoc_server_api_destroy checks for NULL, so it's safe to do that, and less typing. Just a style difference?

I believe this was left over from a previous version of the code. I've updated this and am now calling mongoc_server_api_destroy unconditionally.

src/libmongoc/src/mongoc/mongoc-topology-scanner.c

samantharitter · 2021-03-17T15:55:45Z

src/libmongoc/src/mongoc/mongoc-topology-scanner.c

@@ -399,6 +410,10 @@ mongoc_topology_scanner_destroy (mongoc_topology_scanner_t *ts)
   bson_destroy (&ts->ismaster_cmd_with_handshake);
   bson_destroy (&ts->cluster_time);

+   if (ts->api) {
+      mongoc_server_api_destroy (ts->api);


This doesn't need to be in an if statement unless you prefer it, mongoc_server_api_destroy is happy to accept NULL without breaking.

src/libmongoc/src/mongoc/mongoc-topology-scanner.c

samantharitter · 2021-03-17T16:02:01Z

src/libmongoc/src/mongoc/mongoc-cmd.c

@@ -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. */


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

samantharitter · 2021-03-17T16:06:41Z

src/libmongoc/tests/test-mongoc-topology.c

+   ismaster = bson_strdup_printf ("{'ok': 1,"
+                                  " 'ismaster': true,"
+                                  " 'setName': 'rs',"
+                                  "  'minWireVersion': 2,"


super nit, weird spacing

Copypasta strikes again. I've aligned the indentation across all occurrences in the file.

samantharitter · 2021-03-17T16:10:17Z

src/libmongoc/tests/test-mongoc-versioned-api.c

+   ASSERT_ERROR_CONTAINS (error,
+                          MONGOC_ERROR_CLIENT,
+                          MONGOC_ERROR_CLIENT_API_ALREADY_SET,
+                          "Cannot set server api more than once");


This is interesting. So, when we get the client from the pool, it does not have an api set (client->api is NULL) but we've set the client->api_set flag to prevent users from setting the api directly on the client? It's a bit of a misleading message, I would rather that the message said something like "can't set server api directly on a pooled client, please set it on the parent pool"

As per above, I'll change the flag name to is_pooled and will check that for pooled client. This makes the flag reusable in CDRIVER-3537 if we decide to block setting other properties.

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

bazile-clyde

Looks good. Just a few comments.

src/libmongoc/src/mongoc/mongoc-cmd.c

src/libmongoc/src/mongoc/mongoc-topology-scanner.c

src/libmongoc/tests/test-mongoc-topology.c

samantharitter

I have one question about the pool->client_initialized flag, but the rest of the changes look good!

samantharitter · 2021-03-22T15:48:13Z

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

@@ -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;


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

samantharitter · 2021-03-22T15:52:18Z

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

@@ -528,6 +531,15 @@ mongoc_client_pool_set_server_api (mongoc_client_pool_t *pool,
      return false;
   }

+   if (pool->client_initialized) {


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.

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.

alcaeus requested a review from samantharitter March 11, 2021 13:08

alcaeus self-assigned this Mar 11, 2021

alcaeus commented Mar 11, 2021

View reviewed changes

src/libmongoc/src/mongoc/mongoc-client-pool.c Show resolved Hide resolved

alcaeus commented Mar 11, 2021

View reviewed changes

alcaeus force-pushed the cdriver-3929 branch 2 times, most recently from 507f78b to 984d5f6 Compare March 16, 2021 09:19

alcaeus requested a review from bazile-clyde March 16, 2021 10:26

samantharitter suggested changes Mar 17, 2021

View reviewed changes

bazile-clyde reviewed Mar 17, 2021

View reviewed changes

src/libmongoc/src/mongoc/mongoc-client-pool.c Outdated Show resolved Hide resolved

alcaeus requested review from samantharitter and bazile-clyde March 18, 2021 09:06

bazile-clyde reviewed Mar 18, 2021

View reviewed changes

src/libmongoc/src/mongoc/mongoc-cmd.c Show resolved Hide resolved

src/libmongoc/src/mongoc/mongoc-topology-scanner.c Show resolved Hide resolved

src/libmongoc/tests/test-mongoc-topology.c Outdated Show resolved Hide resolved

alcaeus added 8 commits March 19, 2021 09:28

Fix tab indentation

76e2cf0

CDRIVER-3929 Include apiVersion in handshake commands

4a5ff51

Protect pooled clients against later API changes

1abc83e

Fix wrong indentation

4a293c6

Fix wrong re-initialisation of BSON objects

2458ca5

Unconditionally destroy ts->api

59df965

Improve handling of server API on pooled clients

2b40720

Code review fixes

f149b68

alcaeus force-pushed the cdriver-3929 branch from 646ebed to f149b68 Compare March 19, 2021 08:44

bazile-clyde approved these changes Mar 22, 2021

View reviewed changes

samantharitter reviewed Mar 22, 2021

View reviewed changes

jmikola mentioned this pull request Mar 22, 2021

PHPC-1716 Allow configuring server API version in manager mongodb/mongo-php-driver#1204

Merged

benjirewis mentioned this pull request Mar 23, 2021

Sync Versioned API Tests #757

Merged

alcaeus merged commit 6c6935c into mongodb:master Mar 25, 2021

alcaeus deleted the cdriver-3929 branch March 25, 2021 08:16

CDRIVER-3929 Include apiVersion in handshake commands #753

CDRIVER-3929 Include apiVersion in handshake commands #753

Uh oh!

Conversation

alcaeus commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samantharitter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bazile-clyde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samantharitter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alcaeus commented Mar 11, 2021 •

edited

Loading

alcaeus Mar 24, 2021 •

edited

Loading