Skip to content

CDRIVER-6080 track client streams in connection counting tests #2095

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

kevinAlbs
Copy link
Collaborator

Summary

This PR adds a test utility to count client-side streams: stream_tracker_t.

Tests in CDRIVER-6080 are unskipped and updated to replace server-side connection tracking with the client-side stream tracking.

As a drive-by fix, the llvm-symbolizer path is updated to prefer /opt/mongodbtoolchain/v4 if available.

Motivation

Running only /client_pool/disconnects_removed_servers/on_push on 100x-repeat reproduced failures: https://spruce.mongodb.com/version/689f2acc28a3590007ece216:

test error in: ../../../src/libmongoc/tests/test-mongoc-client-pool.c 445:disconnects_removed_servers_on_push()
Got unexpected connection count to localhost:27017:
Expected 6, got 4

With fixes from this PR, the test consistently appears to pass: https://spruce.mongodb.com/version/68a1fc25c3e9fc00072bbb70.

The test previously used the server-reported connections.current from serverStatus. Quoting docs:

The value will include all incoming connections including any shell connections or connections from other servers

I suspect connections.current might fluctuate from internal connections or (possibly) other connecting host processes. Testing locally, I saw connections.current increase after adding a replica set member, then later decrease after some time passed. And I am unsure if other processes on a host might be connecting.

llvm-symbolizer path

Updating the llvm-symbolizer path was motivated by an observed failure to report a leak: https://spruce.mongodb.com/version/68a0851cae97e00007709c1a:

llvm-symbolizer: Unknown command line argument '--inlines'.  Try: '/opt/mongodbtoolchain/v3/bin/llvm-symbolizer -help'
llvm-symbolizer: Did you mean '-inlining'?

After updated llvm-symbolizer path: https://spruce.mongodb.com/version/68a1fb780780030007cac342:

==10291==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 5824 byte(s) in 1 object(s) allocated from:
[...]

Update tests checking server-side connections. Intended to fix flaky failures.
@kevinAlbs kevinAlbs requested a review from eramongodb August 18, 2025 20:22
@kevinAlbs kevinAlbs marked this pull request as ready for review August 18, 2025 20:22
@kevinAlbs kevinAlbs requested a review from a team as a code owner August 18, 2025 20:22
To fix IPv4/IPv6 tests that use IP literals
Accidentally removed during rebase
Comment on lines -1243 to -1259
bson_t *cmd = BCON_NEW("serverStatus", BCON_INT32(1));
bson_t reply;
bson_error_t error;
bool ok = mongoc_client_command_simple(client, "admin", cmd, NULL, &reply, &error);
if (!ok) {
printf("serverStatus failed: %s\n", error.message);
abort();
}
int32_t conns;
// Get `connections.current` from the reply.
{
bson_iter_t iter;
BSON_ASSERT(bson_iter_init_find(&iter, &reply, "connections"));
BSON_ASSERT(bson_iter_recurse(&iter, &iter));
BSON_ASSERT(bson_iter_find(&iter, "current"));
conns = bson_iter_int32(&iter);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a similar function which uses connections.totalCreated. Should this also be updated to use the new stream tracker?

void
stream_tracker_track_pool(stream_tracker_t *st, mongoc_client_pool_t *pool);

unsigned
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unsigned
int

Avoid unsigned integers for counting. (CppCoreGuidelines)

}


mongoc_stream_t *
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mongoc_stream_t *
static mongoc_stream_t *

Internal linkage.

expect, \
_got); \
} \
mlib_sleep_for(10, ms); \
Copy link
Contributor

Choose a reason for hiding this comment

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

10ms is pretty short given a 5 seconds timeout. Perhaps use 100ms or 1s instead?


typedef struct {
mongoc_host_list_t host;
unsigned count;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unsigned count;
int count;

Avoid unsigned integers for counting. (CppCoreGuidelines)

Comment on lines 1318 to 1319
stream_tracker_assert_count(st, "localhost.test.build.10gen.cc:27017", 0u);
stream_tracker_assert_count(st, "localhost.test.build.10gen.cc:27018", 0u);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stream_tracker_assert_count(st, "localhost.test.build.10gen.cc:27017", 0u);
stream_tracker_assert_count(st, "localhost.test.build.10gen.cc:27018", 0u);
stream_tracker_assert_count(st, host0, 0u);
stream_tracker_assert_count(st, host1, 0u);

These hostnames should be made variables due to repeated reuse.

Comment on lines 408 to 409
stream_tracker_assert_count(st, "localhost:27017", 0u);
stream_tracker_assert_count(st, "localhost:27018", 0u);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stream_tracker_assert_count(st, "localhost:27017", 0u);
stream_tracker_assert_count(st, "localhost:27018", 0u);
stream_tracker_assert_count(st, host0, 0u);
stream_tracker_assert_count(st, host1, 0u);

These hostnames should be made variables due to repeated reuse.

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.

2 participants