Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
10 changes: 10 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-change-stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,7 @@ mongoc_change_stream_next(mongoc_change_stream_t *stream, const bson_t **bson)
}

resumable = _is_resumable_error(stream, err_doc);
int iteration_timeout_count = 0;
while (resumable) {
/* recreate the cursor. */
mongoc_cursor_destroy(stream->cursor);
Expand All @@ -523,9 +524,18 @@ mongoc_change_stream_next(mongoc_change_stream_t *stream, const bson_t **bson)
}
if (err_doc) {
resumable = _is_resumable_error(stream, err_doc);
if (stream->cursor->had_stream_timeout) {
iteration_timeout_count++;
}
} else {
resumable = false;
}

if (iteration_timeout_count >= 2) {
// CDRIVER-6182: Do not resume if two iteration timeouts occur. Intended to avoid a possible resume loop
// when `aggregate` succeeds but `getMore` consistently times out.
resumable = false;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (err_doc) {
resumable = _is_resumable_error(stream, err_doc);
if (stream->cursor->had_stream_timeout) {
iteration_timeout_count++;
}
} else {
resumable = false;
}
if (iteration_timeout_count >= 2) {
// CDRIVER-6182: Do not resume if two iteration timeouts occur. Intended to avoid a possible resume loop
// when `aggregate` succeeds but `getMore` consistently times out.
resumable = false;
}
if (err_doc) {
if (stream->cursor->had_stream_timeout) {
iteration_timeout_count++;
}
// CDRIVER-6182: Do not resume if two iteration timeouts occur. Intended to avoid a possible resume loop
// when `aggregate` succeeds but `getMore` consistently times out.
if (iteration_timeout_count >= 2) {
resumable = false;
} else {
resumable = _is_resumable_error(stream, err_doc);
}
} else {
resumable = false;
}

Suggest tweaking the layout of blocks as suggested to consistently ensure resumable is set once the if (err_doc) block is evaluated by moving each resumable = ... to the end of every possible branch.

@eramongodb eramongodb Jan 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One more suggestion: perhaps the CDRIVER-6182 branch should include a MONGOC_WARNING to notify users when this scenario is triggered?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Suggest tweaking the layout

This led me to realize the if(err_doc) check was redundant. This is only entered if mongoc_cursor_error_document previously returned true. mongoc_cursor_error_document guarantees:

If the function returns true and reply is not NULL, then reply is set to a pointer to a BSON document, which is either empty or the server’s error response.

Changed if (err_doc) to BSON_ASSERT (err_doc) and also applied the suggested tweaking to use if/else so resumable is only set once.

One more suggestion

I like that idea. Added a warning.

}

if (!resumable) {
Expand Down
1 change: 1 addition & 0 deletions src/libmongoc/src/mongoc/mongoc-cluster.c
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ _handle_network_error(mongoc_cluster_t *cluster, mongoc_server_stream_t *server_
type = MONGOC_SDAM_APP_ERROR_NETWORK;
if (mongoc_stream_timed_out(server_stream->stream)) {
type = MONGOC_SDAM_APP_ERROR_TIMEOUT;
server_stream->timed_out = true;
}

_mongoc_topology_handle_app_error(topology,
Expand Down
3 changes: 2 additions & 1 deletion src/libmongoc/src/mongoc/mongoc-cursor-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ struct _mongoc_cursor_t {
uint32_t dblen;

bson_error_t error;
bson_t error_doc; /* always initialized, and set with server errors. */
bson_t error_doc; /* always initialized, and set with server errors. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
bson_t error_doc; /* always initialized, and set with server errors. */
bson_t error_doc; // always initialized, and set with server errors.

Local comment style consistency.

bool had_stream_timeout; // True if previous command run resulted in a timeout on the mongoc_stream_t.

const bson_t *current;

Expand Down
4 changes: 4 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-cursor.c
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,8 @@ _mongoc_cursor_run_command(

ENTRY;

cursor->had_stream_timeout = false; // Reset before running next command.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest moving further below just before the if (parts.assembled.session) block to group together with cursor-state-modifying operations, e.g. cursor->client_session = ...;, cursor->explicit_session = ...;, etc.


const char *cmd_name = _mongoc_get_command_name(command);

mongoc_cmd_parts_init(&parts, cursor->client, db, MONGOC_QUERY_NONE, command);
Expand Down Expand Up @@ -835,6 +837,8 @@ _mongoc_cursor_run_command(
memset(&cursor->error, 0, sizeof(bson_error_t));
}

cursor->had_stream_timeout = server_stream->timed_out;

if (is_retryable && _mongoc_read_error_get_type(ret, &cursor->error, reply) == MONGOC_READ_ERR_RETRY) {
is_retryable = false;

Expand Down
1 change: 1 addition & 0 deletions src/libmongoc/src/mongoc/mongoc-server-stream-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ typedef struct _mongoc_server_stream_t {
// by a network error establishing an initial connection. Used to avoid
// further retry attempts.
bool retry_attempted;
bool timed_out; // True if an operation on `stream` timed out.
} mongoc_server_stream_t;


Expand Down
1 change: 1 addition & 0 deletions src/libmongoc/src/mongoc/mongoc-server-stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ mongoc_server_stream_new(const mongoc_topology_description_t *td,
server_stream->stream = stream; /* merely borrowed */
server_stream->must_use_primary = false;
server_stream->retry_attempted = false;
server_stream->timed_out = false;

return server_stream;
}
Expand Down
3 changes: 2 additions & 1 deletion src/libmongoc/tests/test-conveniences.c
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,8 @@ match_json(const bson_t *doc,
}

ctx.is_command = is_command;
matches = match_bson_with_ctx(doc, pattern, &ctx);
bson_t empty = BSON_INITIALIZER;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Drive-by fix to support NULL for doc and matches the comment above this function:

A NULL doc or NULL json_pattern means "{}".

matches = match_bson_with_ctx(doc ? doc : &empty, pattern, &ctx);

if (!matches) {
char *as_string = doc ? bson_as_canonical_extended_json(doc, NULL) : NULL;
Expand Down
Loading