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

Jump to bottom

test(NODE-6631): Implement integration tests for improved client.close() - sockets + timers #4361

Open

aditi-khare-mongoDB wants to merge 51 commits into main from NODE-6620/sockets

+386 −37

Contributor

aditi-khare-mongoDB commented Dec 30, 2024 •

edited

Loading

Description

Add socket integration tests for client.close().

What is changing?

Assert socket creation and clean-up through libuv tracking for each relevant driver instance of socket creation.
Assert timer creation by asserting timers are defined on relevant client components and assert timer clean-up through process.getActiveResourceInfo()
- Timer existence is more finicky to test, and require some artificial delaying of promises and high timer MSs
- ex: a high heartbeatFrequencyMS to ensure we're not in between intervals when we assert MonitorInterval's timer exits

To see more information regarding testing plan outline see the design document, specifically the dropdowns under Sockets and Timers sections, respectively.

Is there new documentation needed for these changes?

No

What is the motivation for this change?

Improved client.close()

Release Highlight

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

aditi-khare-mongoDB and others added 23 commits

December 16, 2024 14:19


          skeleton

0af55e9


          skeleton updates

4798bc2


          refactor for table

8adca00


          boilerplate execution

3412bb1

fix

1531bec


          preliminary tests finished

4200f3f


          remove misc file

321ef71


          temp for screen-share


          lint

a128974


          boilerplate fully working

4e55dee


          TLS test case running

f560088


          clea up lint

ef9cc90


          clean up

9ed1528


          most of tree reformatting done

1c8e20f


          reorganized tests and added in most of neal's suggestions

1de9809


          TLS test cases and socket test cases

e77405f


          TLS test cases

11aa73c


          fix message formatting

c99579a


          Merge branch 'main' into NODE-6615/integration-client-close

1aeb046


          fix message formatting + test cases naming

47c20db


          Delete logs.txt

133b20d


          requested changes: remove log calls, change chai to expect, clarify s…

8fd53a4

…cript vars/functions


          requested changes: additional expectation

e60a42b

evergreen-ci-prod bot commented Dec 30, 2024

There is an existing patch(es) for this commit SHA:

Evergreen patch with PR

Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'.

aditi-khare-mongoDB changed the base branch from main to NODE-6620/implement-client-close-test

December 30, 2024 20:39

aditi-khare-mongoDB changed the base branch from NODE-6620/implement-client-close-test to NODE-6615/integration-client-close

December 30, 2024 20:39

aditi-khare-mongoDB changed the title ~~test(NODE-6620): Add Improved Client.close() Socket Resource Integration Tests~~ test(NODE-6620): Implement integration tests for improved client.close() - Socket

aditi-khare-mongoDB changed the title ~~test(NODE-6620): Implement integration tests for improved client.close() - Socket~~ test(NODE-6620): Implement integration tests for improved client.close() - Sockets


          temp

13a7f27

aditi-khare-mongoDB force-pushed the NODE-6620/sockets branch from 617e9af to 13a7f27 Compare

December 30, 2024 21:23

aditi-khare-mongoDB added 3 commits

January 3, 2025 11:08


          temp

9ec5b00


          neal's requested changes

5eb92ee


          remove extra files

78f787f

aditi-khare-mongoDB force-pushed the NODE-6620/sockets branch from 54579bd to 78f787f Compare

January 5, 2025 17:20

aditi-khare-mongoDB and others added 4 commits

January 5, 2025 12:22


          Merge branch 'NODE-6615/integration-client-close' into NODE-6620/sockets

125305a


          re-add assertion

77fadbb


          Update test/integration/node-specific/resource_tracking_script_builde…

aefa1cc

…r.ts

Co-authored-by: Anna Henningsen <[email protected]>


          temp

2a43e4d

aditi-khare-mongoDB force-pushed the NODE-6620/sockets branch from 028bf63 to 2a43e4d Compare

January 6, 2025 19:15

aditi-khare-mongoDB and others added 4 commits

January 6, 2025 14:17


          make stderr inherit

e9a3108


          sockets working

e96a94c


          timers

d828529


          Merge branch 'NODE-6615/integration-client-close' into NODE-6620/sockets

1f9f0ba

aditi-khare-mongoDB changed the title ~~test(NODE-6631): Implement integration tests for improved client.close() - Sockets + Timers~~ test(NODE-6631): Implement integration tests for improved client.close() - sockets + timers

aditi-khare-mongoDB commented

View reviewed changes

test/tools/fixtures/process_resource_script.in.js

-                    !originalReportAddresses.includes(resource.address) &&
-                    resource.is_referenced && // if a resource is unreferenced, it's not keeping the event loop open
-                    (!serverType.includes(resource.type) || resource.is_active)
+                    !originalReportAddresses.includes(resource.address) && resource.is_referenced // if a resource is unreferenced, it's not keeping the event loop open

Contributor Author

aditi-khare-mongoDB Jan 7, 2025 •

edited

Loading

Follow-up from NODE-6620: We do want to clean up inactive sockets per Slack discussion

test/integration/node-specific/client_close.test.ts Outdated


		const servers = client.topology?.s.servers;

		// note: minPoolSizeCheckFrequencyMS = 100 ms by client, so this test has a chance of being flaky

Contributor Author

aditi-khare-mongoDB Jan 7, 2025

See the note on the comment. This test has a higher chance of being flaky since the client can't set minPoolSizeCheckFrequencyMS. I can this to set the variable to a higher value using sinon if preferred by reviewers.


          eod commit

24ebcde

Base automatically changed from NODE-6615/integration-client-close to main

January 8, 2025 16:18


          rebase

f003454

rebase

aditi-khare-mongoDB force-pushed the NODE-6620/sockets branch from 9793c44 to f003454 Compare

January 9, 2025 07:59


          Merge branch 'main' into NODE-6620/sockets

aditi-khare-mongoDB force-pushed the NODE-6620/sockets branch 2 times, most recently from e97cc3c to 4865259 Compare

January 10, 2025 18:27


          push for virtual office

59fda80

updated tests

delete extraneous files

srv test

lint

aditi-khare-mongoDB force-pushed the NODE-6620/sockets branch from 4865259 to 59fda80 Compare

January 10, 2025 18:27


          Merge branch 'main' into NODE-6620/sockets

a79ed0a

aditi-khare-mongoDB marked this pull request as ready for review

January 10, 2025 18:31

nbbeeken self-assigned this

nbbeeken added the Primary Review label

nbbeeken self-requested a review

January 10, 2025 19:35

nbbeeken requested changes

View reviewed changes

test/integration/node-specific/client_close.test.ts

+                          let serverSelectionTimeoutStarted = false;
+                          // make server selection hang so check out timer isn't cleared and check that the timeout has started
+                          sinon.stub(Promise, 'race').callsFake(async ([_serverPromise, timeout]) => {

Contributor

nbbeeken Jan 10, 2025

If we test against a replicaset and use a ReadPreference that doesn't match any servers then we can be sure we're stuck in server selection until the timer fires. Then we won't need to sinon spy or stub here.

This is how I'm testing server selection interruption for signal support:
https://github.com/mongodb/node-mongodb-native/blob/NODE-6258-abortsignal/test/integration/node-specific/abort_signal.test.ts#L201-L244

I'm also seeing an error when I skip over the resource assertions:

TypeError: Cannot read properties of undefined (reading 'setTimeout')

test/integration/node-specific/client_close.test.ts

Comment on lines +142 to +144

+                                  client.on('serverHeartbeatSucceeded', () => (heartbeatHappened = true));
+                                  await client.connect();
+                                  await sleep(heartbeatFrequencyMS * 2.5);

Contributor

nbbeeken Jan 10, 2025

What if we awaited the 'serverHeartbeatSucceeded' event directly instead of sleeping for some amount of time?

test/integration/node-specific/client_close.test.ts

Comment on lines +147 to +151

+                                  function getMonitorTimer(servers) {
+                                    for (const server of servers) {
+                                      return server[1]?.monitor.monitorId.timerId;
+                                    }
+                                  }

Contributor

nbbeeken Jan 10, 2025

what about topologies other than standalone should we be returning more than the first timer?

test/integration/node-specific/client_close.test.ts

+                                  const servers = client.topology.s.servers;
+                                  expect(getMonitorTimer(servers)).to.exist;
+                                  await client.close();
+                                  expect(getMonitorTimer(servers)).to.not.exist;

Contributor

nbbeeken Jan 10, 2025

I don't feel strongly but I don't think we need to check that it has been set to null/undefined because if it was left on there it wouldn't be wrong as long as it is still cleared in the "resource" sense.

test/integration/node-specific/client_close.test.ts

Comment on lines +169 to +170

		/* eslint-disable-next-line @typescript-eslint/no-unused-vars */
		const run = async function ({ MongoClient, uri, expect, sleep, getTimerCount }) {

Contributor

nbbeeken Jan 10, 2025

The nice part about giving all the inputs as an object that we can pick and chose the inputs by simply omitting the dependencies we aren't using. So if you remove uri then you can remove the lint exemption

test/integration/node-specific/client_close.test.ts

+                              };
+                              await utilClient.db('admin').command(failPoint);
+                              const timeoutStartedSpy = sinon.spy(timers, 'setTimeout');

Contributor

nbbeeken Jan 10, 2025

I'm seeing an error thrown for this the same way it was for the server selection test. I don't think we need the spy if we know we have a commandStarted on a client with maxPoolSize: 1

test/integration/node-specific/client_close.test.ts

+                              const timeoutStartedSpy = sinon.spy(timers, 'setTimeout');
+                              const client = new MongoClient(uri, {
+                                minPoolSize: 1,

Contributor

nbbeeken Jan 10, 2025

I would only set the max so that we don't create the timer for satisfying minPoolSize, not necessarily a problem but more resources that we need to make for this test.

test/integration/node-specific/client_close.test.ts


		await utilClient.close();

		const err = await insertPromise;

Contributor

nbbeeken Jan 10, 2025

Shouldn't the first insertPromise succeed (just after a long time), its the second one that won't be able to complete connection checkout

Contributor Author

aditi-khare-mongoDB Jan 10, 2025

I was confused / surprised about this as well. If we await both of them, the first one fails and the second one succeeds.

test/integration/node-specific/client_close.test.ts

+                                  process.report.getReport().libuv.filter(r => r.type === 'tcp');
+                                // assert no sockets to start with
+                                expect(connectionMonitoringReport()).to.have.length(0);

Contributor

nbbeeken Jan 10, 2025

Suggested change

      
                              expect(connectionMonitoringReport()).to.have.length(0);
          
                              expect(connectionMonitoringReport()).to.have.lengthOf(0);

I could be wrong but I think you have to use lengthOf

test/integration/node-specific/client_close.test.ts

                               const infiniteFile = '/dev/zero';
-                              const kmsProviders = BSON.EJSON.parse(process.env.CSFLE_KMS_PROVIDERS);
+                              const kmsProviders = mongodb.BSON.EJSON.parse(process.env.CSFLE_KMS_PROVIDERS);

Contributor

nbbeeken Jan 10, 2025

This test fails locally when CSFLE_KMS_PROVIDERS isn't defined, which is the default, can you put the metadata on this test to have it skip unless encryption is enabled?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels