Skip to content

CDRIVER-4184 don't emit closed on invalid topology #884

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.

Sign up for GitHub

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 2 commits into from
Nov 2, 2021
Merged

CDRIVER-4184 don't emit closed on invalid topology #884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a72090
CDRIVER-4184 don't emit closed on invalid topology
kevinAlbs Oct 30, 2021
1dfb37f
add comments
kevinAlbs Nov 1, 2021
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
7 changes: 6 additions & 1 deletion src/libmongoc/src/mongoc/mongoc-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,12 @@ mongoc_topology_destroy (mongoc_topology_t *topology)
bson_mutex_destroy (&topology->apm_mutex);
mongoc_cond_destroy (&topology->srv_polling_cond);
}
_mongoc_topology_description_monitor_closed (&topology->description);

if (topology->valid) {
Copy link
Contributor

@chardan chardan Oct 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

+1 I'm assuming that this is okay via spec. (Also, re-read the bug report makes sense! Thank you.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The SDAM monitoring spec defines the TopologyOpening event as:

When a topology description is initialized - this MUST be the first SDAM event fired

I interpret that to mean it is OK to omit a TopologyOpening event if the topology fails to initialize. That is the current behavior. And I think it follows that a TopologyClosed event must not be omitted either.

/* Do not emit a topology_closed event. A topology opening event was not
* emitted. */
_mongoc_topology_description_monitor_closed (&topology->description);
}

mongoc_uri_destroy (topology->uri);
mongoc_topology_description_cleanup (&topology->description);
Expand Down
137 changes: 137 additions & 0 deletions src/libmongoc/tests/test-mongoc-dns.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,127 @@ prose_test_9_pooled (void *unused)
_prose_test_9 (true);
}

/* cb_stats_t tracks counters for the test_invalid_topology_pooled and
* test_invalid_topology_single tests. */
typedef struct {
int num_topology_opening;
int num_topology_closed;
int num_server_opening;
int num_server_closed;
} cb_stats_t;

/* invalid_topology_opening is used as a callback for the
* test_invalid_topology_pooled and test_invalid_topology_single tests. */
static void
invalid_topology_opening (const mongoc_apm_topology_opening_t *event) {
cb_stats_t *stats = (cb_stats_t*) mongoc_apm_topology_opening_get_context (event);
stats->num_topology_opening++;
}

/* invalid_topology_closed is used as a callback for the
* test_invalid_topology_pooled and test_invalid_topology_single tests. */
static void
invalid_topology_closed (const mongoc_apm_topology_closed_t *event) {
cb_stats_t *stats = (cb_stats_t*) mongoc_apm_topology_closed_get_context (event);
stats->num_topology_closed++;
}

/* invalid_server_closed is used as a callback for the
* test_invalid_topology_pooled and test_invalid_topology_single tests. */
static void
invalid_server_closed (const mongoc_apm_server_closed_t *event) {
cb_stats_t *stats = (cb_stats_t*) mongoc_apm_server_closed_get_context (event);
stats->num_server_closed++;
}

/* invalid_server_opening is used as a callback for the
* test_invalid_topology_pooled and test_invalid_topology_single tests. */
static void
invalid_server_opening (const mongoc_apm_server_opening_t *event) {
cb_stats_t *stats = (cb_stats_t*) mongoc_apm_server_opening_get_context (event);
stats->num_server_opening++;
}

/* CDRIVER-4184 Test that an invalid topology does not emit a topology_closed
* event. */
static void
test_invalid_topology_pooled (void *unused) {
mongoc_client_pool_t *pool;
mongoc_client_t *client;
mongoc_uri_t *uri;
mongoc_apm_callbacks_t *cbs;
cb_stats_t stats = {0};

/* TXT record for test20.test.build.10gen.cc resolves to "loadBalanced=true". */
uri = mongoc_uri_new ("mongodb+srv://test20.test.build.10gen.cc/?replicaSet=rs0");
pool = mongoc_client_pool_new (uri);
cbs = mongoc_apm_callbacks_new ();
mongoc_apm_set_topology_opening_cb (cbs, invalid_topology_opening);
mongoc_apm_set_topology_closed_cb (cbs, invalid_topology_closed);
mongoc_apm_set_server_opening_cb (cbs, invalid_server_opening);
mongoc_apm_set_server_closed_cb (cbs, invalid_server_closed);
mongoc_client_pool_set_apm_callbacks (pool, cbs, &stats);

ASSERT_CMPINT(stats.num_topology_opening, ==, 0);

/* Pop a client to attempt to start monitoring. Monitoring emits the topology_opening event on valid topologies. */
client = mongoc_client_pool_pop (pool);
mongoc_client_pool_push (pool, client);

mongoc_apm_callbacks_destroy (cbs);
mongoc_client_pool_destroy (pool);
mongoc_uri_destroy (uri);

ASSERT_CMPINT(stats.num_topology_opening, ==, 0);
ASSERT_CMPINT(stats.num_server_opening, ==, 0);
ASSERT_CMPINT(stats.num_server_closed, ==, 0);
ASSERT_CMPINT(stats.num_topology_closed, ==, 0);
}

/* CDRIVER-4184 Test that an invalid topology does not emit a topology_closed
* event. */
static void
test_invalid_topology_single (void *unused) {
mongoc_client_t *client;
mongoc_uri_t *uri;
mongoc_apm_callbacks_t *cbs;
cb_stats_t stats = {0};
bson_error_t error;
mongoc_server_description_t *sd;

/* TXT records for test20.test.build.10gen.cc resolve to loadBalanced=true. */
uri = mongoc_uri_new ("mongodb+srv://test20.test.build.10gen.cc/?replicaSet=true");
client = mongoc_client_new_from_uri (uri);
cbs = mongoc_apm_callbacks_new ();
mongoc_apm_set_topology_opening_cb (cbs, invalid_topology_opening);
mongoc_apm_set_topology_closed_cb (cbs, invalid_topology_closed);
mongoc_apm_set_server_opening_cb (cbs, invalid_server_opening);
mongoc_apm_set_server_closed_cb (cbs, invalid_server_closed);
mongoc_client_set_apm_callbacks (client, cbs, &stats);

ASSERT_CMPINT(stats.num_topology_opening, ==, 0);

/* Perform server selection. Server selection emits the topology_opening
* event on valid topologies. */
sd = mongoc_client_select_server (
client, false /* for_writes */, NULL /* read_prefs */, &error);
ASSERT_ERROR_CONTAINS (
error,
MONGOC_ERROR_SERVER_SELECTION,
MONGOC_ERROR_SERVER_SELECTION_FAILURE,
"URI with \"loadbalanced\" enabled must not contain option \"replicaset\"");
ASSERT (!sd);

mongoc_apm_callbacks_destroy (cbs);
mongoc_client_destroy (client);
mongoc_uri_destroy (uri);

ASSERT_CMPINT(stats.num_topology_opening, ==, 0);
ASSERT_CMPINT(stats.num_server_opening, ==, 0);
ASSERT_CMPINT(stats.num_server_closed, ==, 0);
ASSERT_CMPINT(stats.num_topology_closed, ==, 0);
}

void
test_dns_install (TestSuite *suite)
{
Expand Down Expand Up @@ -770,4 +891,20 @@ test_dns_install (TestSuite *suite)
NULL,
NULL,
test_dns_check_loadbalanced);

TestSuite_AddFull (
suite,
"/initial_dns_seedlist_discovery/load-balanced/invalid_topology/pooled",
test_invalid_topology_pooled,
NULL,
NULL,
test_dns_check_loadbalanced);

TestSuite_AddFull (
suite,
"/initial_dns_seedlist_discovery/load-balanced/invalid_topology/single",
test_invalid_topology_single,
NULL,
NULL,
test_dns_check_loadbalanced);
}