Skip to content
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

fix: allow client connect after close #2615

Conversation

HanaPearlman
Copy link
Contributor

This reinstates the work from #2581 now that NODE-2850 is complete.

During close of a MongoClient, we remove the topology. Then, when a client is reconnected, a new topology is created and added to the client. First, this means references to Dbs and Collections can be re-used after the reconnect, since they will pull the new topology from the client. Also, when an operation requiring a topology is performed on a closed client, an error is thrown.

After this, the work of figuring out how to re-connect a topology is one of the last steps towards having a single, immutable topology associated with a MongoClient. As in #2581, I skipped this work for now, since the refactor required to re-connect topologies may be too much work for this ticket.

NODE-2544

@HanaPearlman HanaPearlman marked this pull request as draft November 6, 2020 16:30
@HanaPearlman HanaPearlman marked this pull request as ready for review November 6, 2020 22:42
@@ -618,6 +617,7 @@ function processError(changeStream: ChangeStream, error: AnyError, callback?: Ca
// close internal cursor, ignore errors
cursor.close();

const topology = getTopology(changeStream.parent);
waitForTopologyConnected(topology, { readPreference: cursor.readPreference }, err => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accessing the topology too early in this function causes failing tests when processError is called after client close.

@HanaPearlman HanaPearlman requested review from mbroadst, emadum and nbbeeken and removed request for emadum November 6, 2020 22:48
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

🚀

test/functional/connection.test.js Outdated Show resolved Hide resolved
test/functional/connection.test.js Outdated Show resolved Hide resolved
src/operations/connect.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@HanaPearlman HanaPearlman force-pushed the NODE-2544/master/reconnect-after-close branch from ceb9476 to 4e52381 Compare November 12, 2020 21:20
@HanaPearlman HanaPearlman merged commit 9a176ef into mongodb:master Nov 16, 2020
@HanaPearlman HanaPearlman deleted the NODE-2544/master/reconnect-after-close branch November 16, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants