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.

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

Conversation

kevinAlbs
Copy link
Collaborator

No description provided.

@kevinAlbs kevinAlbs marked this pull request as ready for review October 31, 2021 16:26
@kevinAlbs kevinAlbs requested review from chardan and kmahar October 31, 2021 16:26
Copy link
Contributor

@chardan chardan left a comment

Choose a reason for hiding this comment

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

Looks like a good start! Since it's Halloween today, best check for gremlins, too!

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

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.

Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

thanks @kevinAlbs! I ran a patch with the Swift driver and your branch here vendored in and the DNS seedlist test that was crashing due to this issue now passes.

@kevinAlbs kevinAlbs requested a review from chardan November 1, 2021 22:19
Copy link
Contributor

@chardan chardan left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! I appreciate your clarifications.

@kevinAlbs kevinAlbs merged commit b93e345 into mongodb:master Nov 2, 2021
@kevinAlbs kevinAlbs deleted the loadbalanced-sdam-closed.4184 branch November 2, 2021 21:03
@kevinAlbs kevinAlbs restored the loadbalanced-sdam-closed.4184 branch February 3, 2022 14:29
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.

3 participants