CDRIVER-4617 Refactor libmongoc to use mcd-rpc #1307

eramongodb · 2023-06-13T21:55:38Z

Description

Followup to #1296. This PR replaces mongoc_rpc_t with mcd_rpc_message in libmongoc. This PR deliberately leaves the (now unused) mongoc_rpc_t utilities to facilitate comparison of equivalent utilities and avoid messy diffs. The mongoc_rpc_t utilities will be removed in a followup commit.

Verified by ~~this patch~~ this patch (includes the commit addressing distinct pointer type warnings on Windows).

RPC Utility Functions

There are four RPC message utility functions in libmongoc that require new equivalents:

_mongoc_rpc_op_(egress|ingress)_inc -> mcd_rpc_message_(egress|ingress)
_mongoc_rpc_(compress|decompress) -> mcd_rpc_message_(compress|decompress)
_mongoc_rpc_decompress_if_necessary -> mcd_rpc_message_decompress_if_necessary
_mongoc_rpc_reply_get_first_msg -> mcd_rpc_message_get_body
_mongoc_rpc_check_ok -> mcd_rpc_message_check_ok

The new ingress and egress functions are fairly straightforward. Compared to the old mongoc_rpc_t equivalents which conflated the counters with conversion from data / to iovecs (partially addressed for the egress case by #1256), the mcd_rpc_message functions completely separate conversion (with from_data and to_iovecs) from counter increment (with egress and ingress).

The new compression functions (compress, decompress, and decompress_if_necessary) remove the dependency on mongoc_cluster_t by requesting the compressor level (derived from the cluster's URI) explicitly as a parameter instead. They also do not handle ingress/egress counter increments (as currently being done by the _mongoc_rpc_scatter function) to improve separation of concerns.

The get_first_document function was renamed to get_body to better reflect it's intended use (obtaining the Kind 0 section BSON document of an OP_MSG or the first document in an OP_REPLY). The function now properly handles the possibility of the Kind 0 section not being the first section in the OP_MSG, although this freedom does not appear to have been exercised yet by the server.

The check_ok function is a simple translation and does not require further elaboration.

mongoc_cluster_t::request_id

A drive-by fix to make the type of the request ID consistent with its specification and use.

Mock Server

The new mcd_rpc_message utilities already exposed inconsistent opcode handling of RPC messages that were addressed in #1280 (caught by mcd_rpc_op_query_get_flags in assert_request_matches_flags). This PR contains the changes that led to their discovery. Most of the changes involve improvements to type consistency, greater detail on message parsing failure, and refactors to simplify code (notably, removal of the redundant rpc parameter for the request_from_* functions).

Server Monitor Thread

A fairly straightforward one-to-one translation from mongoc_rpc_t to mcd_rpc_message equivalents. Nearby code was updated to improve readability.

mongoc-async-cmd

Also fairly straightforward one-to-one translation, but removed the redundant mongoc_async_cmd::array data member in favor of local variables to store the iovec structures used for async send operations. The existing mongoc_async_cmd::iovecs data member is used directly instead (now made an owning pointer).

mongoc_cluster_run_command_opquery

Refactored into _mongoc_cluster_run_command_opquery_send and _mongoc_cluster_run_command_opquery_recv. The old implementation used a truly bizarre algorithm when receiving the reply that assumes the message is an OP_REPLY, receiving the incoming bytes as if it is an OP_REPLY, then later realizing it may actually be an OP_COMPRESSED message instead, then reading the remaining bytes of the OP_COMPRESSED message. The implementation was changed to use the more consistent pattern of reading messageLength only first, before consuming all the remaining bytes of the incoming message in one go, then validating the opcode of the entire message, also aided by the much nicer-to-use _mongoc_buffer_append_from_stream interface.

mongoc_cluster_run_opmsg

Also refactored into separate _mongoc_cluster_run_opmsg_send and _mongoc_cluster_run_opmsg_recv functions. It's mostly straightforward translation with the exception of more thorough error-handling checks.

I remain fairly confused by the inconsistent use of RUN_CMD_ERR_DECORATE, _handle_network_error, server_stream->stream = NULL, and network_error_reply (reply, cmd) (including their ordering) in the original code. I would appreciate a closer look into their use to confirm they are being used correctly in the new functions.

Cursor Functions

The only major change of note that isn't straightforward one-to-one translation is the use of local iovecs instead of the mongoc_cluster_t::iov array object, a small refactor of some blocks of code into _mongoc_cursor_op_*_send functions, and improved type consistency.

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

vector-of-bool

LGTM, and the new form is often significantly more readable than the prior version.

My only concern is about validation of received messages. I'm unsure how much of a concern it is, or if there are unseen checks that guard against firing assertions.

src/libmongoc/src/mongoc/mongoc-rpc.c

kevinAlbs

Nicely done. LGTM with minor comments addressed.

kevinAlbs · 2023-06-15T15:43:57Z

src/libmongoc/src/mongoc/mongoc-rpc.c

+      break;
+
+   case MONGOC_OP_CODE_KILL_CURSORS:
+      BSON_UNREACHABLE ("unexpected MONGOC_OP_CODE_CURSORS ingress");


Suggested change

BSON_UNREACHABLE ("unexpected MONGOC_OP_CODE_CURSORS ingress");

BSON_UNREACHABLE ("unexpected MONGOC_OP_KILL_CURSORS ingress");

kevinAlbs · 2023-06-15T17:58:26Z

src/libmongoc/tests/mock_server/request.c


 static void
-request_from_insert (request_t *request, const mongoc_rpc_t *rpc);
+request_from_insert (request_t *request);


Consider removing unused mock server functions for receiving the legacy wire protocol:

mock_server_receives_insert and associated functions

mock_server_receives_bulk_insert and associated functions

mock_server_receives_update and associated functions

mock_server_receives_delete and associated functions
~~- mock_server_receives_getmore and associated functions~~ C driver still uses legacy get more for exhaust cursors in _mongoc_cursor_op_getmore_send.

With the minimum server version of 3.6, I expect the C driver can no longer send any of those protocols. I expect the changes to the mock server for these legacy protocols are not tested.

Even though C Driver still uses legacy OP_GET_MORE, it seems like the mock server receives function is nevertheless not being used anywhere, so will proceed with removing it anyways. Can easily be reverted if we end up needing a test that uses this assertion (hopefully unlikely).

kevinAlbs · 2023-06-15T18:58:42Z

src/libmongoc/src/mongoc/mongoc-cluster.c

+          mcd_rpc_op_compressed_get_compressed_message (rpc),
+          mcd_rpc_op_compressed_get_compressed_message_length (rpc),
+          ptr + message_header_length,
+          &actual_uncompressed_size)) {


Suggested change

&actual_uncompressed_size)) {

&actual_uncompressed_size)) {

bson_free (ptr);

kevinAlbs · 2023-06-16T15:24:52Z

src/libmongoc/src/mongoc/mongoc-cluster.c

+      RUN_CMD_ERR (MONGOC_ERROR_PROTOCOL,
+                   MONGOC_ERROR_PROTOCOL_INVALID_REPLY,
+                   "message length %" PRId32
+                   " is not within valid range of 16-%" PRId32 " bytes",
+                   message_length,
+                   server_stream->sd->max_msg_size);


Suggested change

RUN_CMD_ERR (MONGOC_ERROR_PROTOCOL,

MONGOC_ERROR_PROTOCOL_INVALID_REPLY,

"message length %" PRId32

" is not within valid range of 16-%" PRId32 " bytes",

message_length,

server_stream->sd->max_msg_size);

RUN_CMD_ERR (MONGOC_ERROR_PROTOCOL,

MONGOC_ERROR_PROTOCOL_INVALID_REPLY,

"message length %" PRId32

" is not within valid range of %" PRId32 "-%" PRId32

" bytes",

message_header_length,

message_length,

server_stream->sd->max_msg_size);

Use message_header_length constant.

kevinAlbs · 2023-06-16T15:26:45Z

src/libmongoc/src/mongoc/mongoc-cluster.c

+                      "message length %" PRId32
+                      " is not within valid range of 16-%" PRId32 " bytes",
+                      message_length,
+                      server_stream->sd->max_msg_size);


Use message_header_length:

bson_set_error (error, MONGOC_ERROR_PROTOCOL, MONGOC_ERROR_PROTOCOL_INVALID_REPLY, "message length %" PRId32 " is not within valid range of %" PRId32 " -%" PRId32 " bytes", message_header_length, message_length, server_stream->sd->max_msg_size);

kevinAlbs · 2023-06-16T15:40:38Z

src/libmongoc/src/mongoc/mongoc-cluster.c

+      message_length +=
+         mcd_rpc_header_set_request_id (rpc, ++cluster->request_id);
+      message_length += mcd_rpc_header_set_response_to (rpc, 0);
+      message_length += mcd_rpc_header_set_op_code (rpc, MONGOC_OPCODE_MSG);


Suggested change

message_length += mcd_rpc_header_set_op_code (rpc, MONGOC_OPCODE_MSG);

message_length += mcd_rpc_header_set_op_code (rpc, MONGOC_OP_CODE_MSG);

kevinAlbs · 2023-06-16T15:48:22Z

I remain fairly confused by the inconsistent use of RUN_CMD_ERR_DECORATE,
_handle_network_error, server_stream->stream = NULL, and
network_error_reply (reply, cmd) (including their ordering) in the original
code. I would appreciate a closer look into their use to confirm they are being
used correctly in the new functions.

It looks correct to me.

For some background on server_stream->stream = NULL: I think server_stream->stream = NULL; is to prevent reusing an invalidated mongoc_server_stream_t in functions that use the same mongoc_server_stream_t for multiple commands.
mongoc_cluster_stream_valid is used to determine if a mongoc_server_stream_t is invalid.

A network error may free the mongoc_server_stream_t::mongoc_stream_t:

For single threaded via: _handle_network_error => mongoc_cluster_disconnect_node => mongoc_topology_scanner_node_disconnect.
For pooled via: _handle_network_error => mongoc_cluster_disconnect_node => _mongoc_cluster_node_destroy.

Adding server_stream->stream = NULL to error handling in _mongoc_cluster_run_command_opquery and _mongoc_cluster_run_command_opquery_recv may be a reasonable precaution. But I expect it would not change any observable behavior. OP_QUERY is only used for exhaust cursors and the handshake. Neither use the same stream for multiple commands.

eramongodb · 2023-06-16T17:23:09Z

Latest changes verified by ~~this patch~~ this patch (with merge to address make-release-archive task failure).

eramongodb added 12 commits June 13, 2023 15:57

Add mcd_rpc_message ingress/egress counter functions

9036a70

Add mcd_rpc_message compression functions

8a8e699

Add mcd_rpc_message_get_body

1314c3a

Add mcd_rpc_message_check_ok

48534b2

Refactor mongoc_cluster_t::request_id from uint32_t -> int32_t

ec1fa87

Refactor mock server to use mcd-rpc

9a28052

Refactor server monitor thread to use mcd-rpc

c109dd0

Refactor mongoc-async-cmd to use mcd-rpc

d543399

Refactor mongoc_cluster_run_command_opquery to use mcd-rpc

b2cd256

Refactor mongoc_cluster_run_opmsg to use mcd-rpc

935a510

Refactor cursor functions to use mcd-rpc

eb1b57e

Address distinct pointer type warnings on Windows

935e496

eramongodb requested review from kevinAlbs and vector-of-bool June 13, 2023 21:55

eramongodb self-assigned this Jun 13, 2023

vector-of-bool reviewed Jun 14, 2023

View reviewed changes

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

vector-of-bool approved these changes Jun 14, 2023

View reviewed changes

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

kevinAlbs approved these changes Jun 16, 2023

View reviewed changes

eramongodb added 6 commits June 16, 2023 12:19

Consistently use name of opcode

d4e1bdc

Clean up mock server functions

96b5d69

Fix potential leak in mcd_rpc_message_decompress

83a9b25

Use named constant for header length

1070450

opcode constant consistency

048fbdb

Fix OP_MSG flags signedness

9bbf2a7

eramongodb merged commit 6f58b9d into mongodb:master Jun 16, 2023

eramongodb deleted the cdriver-4617 branch June 16, 2023 18:43

eramongodb mentioned this pull request Jun 16, 2023

CDRIVER-4617 Remove legacy mongoc-rpc entities #1312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CDRIVER-4617 Refactor libmongoc to use mcd-rpc #1307

CDRIVER-4617 Refactor libmongoc to use mcd-rpc #1307

Uh oh!

eramongodb commented Jun 13, 2023 •

edited

Loading

Uh oh!

Uh oh!

vector-of-bool left a comment

Uh oh!

Uh oh!

kevinAlbs left a comment

Uh oh!

kevinAlbs Jun 15, 2023

Uh oh!

kevinAlbs Jun 15, 2023

Uh oh!

eramongodb Jun 16, 2023

Uh oh!

kevinAlbs Jun 15, 2023

Uh oh!

kevinAlbs Jun 16, 2023

Uh oh!

kevinAlbs Jun 16, 2023

Uh oh!

kevinAlbs Jun 16, 2023

Uh oh!

kevinAlbs commented Jun 16, 2023

Uh oh!

eramongodb commented Jun 16, 2023 •

edited

Loading

Uh oh!

Uh oh!

	BSON_UNREACHABLE ("unexpected MONGOC_OP_CODE_CURSORS ingress");
	BSON_UNREACHABLE ("unexpected MONGOC_OP_KILL_CURSORS ingress");

	&actual_uncompressed_size)) {
	&actual_uncompressed_size)) {
	bson_free (ptr);

	message_length += mcd_rpc_header_set_op_code (rpc, MONGOC_OPCODE_MSG);
	message_length += mcd_rpc_header_set_op_code (rpc, MONGOC_OP_CODE_MSG);

CDRIVER-4617 Refactor libmongoc to use mcd-rpc #1307

CDRIVER-4617 Refactor libmongoc to use mcd-rpc #1307

Uh oh!

Conversation

eramongodb commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

RPC Utility Functions

mongoc_cluster_t::request_id

Mock Server

Server Monitor Thread

mongoc-async-cmd

mongoc_cluster_run_command_opquery

mongoc_cluster_run_opmsg

Cursor Functions

Uh oh!

Uh oh!

vector-of-bool left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

eramongodb Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs commented Jun 16, 2023

Uh oh!

eramongodb commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

eramongodb commented Jun 13, 2023 •

edited

Loading

eramongodb commented Jun 16, 2023 •

edited

Loading