test(NODE-5028): assert MongoClients are garbage collectable#3553
test(NODE-5028): assert MongoClients are garbage collectable#3553
Conversation
3a2ee67 to
7dad424
Compare
There was a problem hiding this comment.
I think we can use AsyncResources for this.
If we attach an async resource to each mongo client and enable async hooks, then we can track when the async resources (and by proxy, the MongoClient) are actually garbage collected without relying on measuring the heap size.
baileympearson
left a comment
There was a problem hiding this comment.
CI is pretty red, a number of failures are unrelated (the fle failures on node18 appear on other PRs, I wonder if Node18.14 broke that test)
Should this test be a part of the revert PRs maybe? Otherwise, we can't demonstrate with a test that the revert fixes the memory lead issue, and we can't merge this first because it will fail without the revert.
Also, the async resource tracking always records exactly one client (even with the revert). If we keep the async hooks logic, we'll need to update the assertion (it's currently asserting against 0). But should we keep that logic if we don't know why exactly one mongo client is always reported as being left?
|
We should merge the reverts in isolation for organizational sake, and then we can merge this when it's able to be green. I can post an evg patch showing this running against the driver with the revert |
ba7df9b to
81b3222
Compare
durran
left a comment
There was a problem hiding this comment.
Per @baileympearson 's comment as well - I feel like this is overly complex. Can we remove all the heap size checking and just create multiple MongoClients, run some operations on them and close them and then assert they can be released?
|
We can't be sure that the tracking is accurate because as the test shows it is still showing one MongoClient in memory, while the heap snapshot shows none. So which is the true source of truth? |
Yeah that's really odd and has the feeling it could be a flaky test in the future as well as checking the difference between start and end memory... Maybe we only need the test on line 137 and that's enough? |
|
I do permit a wiggle room of 3 megabytes which is pretty friendly but that was just a guesstimate, we could make the range much larger to avoid flakes. The leak was growing unbounded easily reaching 90+ MB with 100 iterations of client.connect/close so we could do a range of 20MB? it just depends on how much allocation would be beyond our expected amount (relative to startup amount) |
| const mongoClient = new MongoClient(uri); | ||
| await mongoClient.connect(); | ||
| const db = mongoClient.db(); | ||
| await Promise.all([ |
There was a problem hiding this comment.
why db.collections() and why this many times? also, are we interested in ensuring no leak when there is only one client or should we test multiples? (your helper script runs multiple iterations, but I am trying to understand what the actual scenario under test is)
There was a problem hiding this comment.
The operation is arbitrary, the parallelism is to get "some" number of connections in the connection pool, this bug does reproduce without this though, I can make it one db.collections call
There was a problem hiding this comment.
ok but why db.collections? why isn't the connect call enough? if you just want connections in the connection pool, then setting a minpoolsize feels like a more straightforward approach
There was a problem hiding this comment.
I minimized the reproduction more, it's a connect and ping, although I guess the connect is redundant with any operation. I tried only setting a minPoolSize, but the leak stopped reproducing so it would seem that running an operation over the connections is required.
@baileympearson FYI ^
|
|
||
| // If any of the above throws we won't reach these unlinks that clean up the created files. | ||
| // This is intentional so that when debugging the file will still be present to check it for errors | ||
| // await unlink(scriptName); |
There was a problem hiding this comment.
accidentally commented out?
Co-authored-by: Daria Pardue <daria.pardue@mongodb.com>
Description
Using both self process memory reporting and heapsnapshots, we can check that after 100 clients have been created and closed they are removed from memory by the GC.
What is changing?
Is there new documentation needed for these changes?
What is the motivation for this change?
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript