fix(sessions): move active session tracking to topology base #1665
Conversation
daprahamian
force-pushed
the
HELP-5384
branch
from
53cba03
to
6dde985
Compare
Moves the tracking of active sessions to the topology base. Doing this allows us to ensure that all active and pooled sessions are ended when the topology closes, and that implicit sessions are tracked. Part of HELP-5384
daprahamian
force-pushed
the
HELP-5384
branch
from
6dde985
to
1f1c7c3
Compare
daprahamian
changed the title
…essions A hook that executes both during and after each test, ensuring that no sessions are leaked. This will cause A LOT of failures, since anything that does not properly call client.close will register as a leak.
Crud Spec tests were never calling client.close
Tests were not calling client.close().
This test doesn't do anything. And I have a feeling that with our new approach to options management, we don't want it to do what it thinks it should be doing
Fixing tests in generator examples that do not properly call client.close. Still need to fix leak in rename exmaple
Fixing tests in promise examples that do not properly call client.close. Still need to fix leak in rename exmaple
Fixing tests in cursor tests that do not properly call client.close.
Fixing tests in db tests that do not properly call client.close.
Fixes tests in operation examples that do not properly call client.close Still need to fix actively leaking sessions
These sessions will be cleaned up by close, but to make the session leak tests pass, we manually close these sessions
daprahamian
force-pushed
the
HELP-5384
branch
from
c7af865
to
a791ed4
Compare
Part of this involves adding the ability to skip tests in session leak tests
These tests cannot pass session leak testing until after NODE-1335 is finished
| }); | ||
|
|
||
| // We need to wash out all stored processes | ||
| if (forceClosed === true) { |
There was a problem hiding this comment.
hmm doesn't look like this code made it up to the base class, was this accidentally removed?
There was a problem hiding this comment.
That code was already here
test/functional/index_tests.js
Outdated
|
|
||
| client.close(); | ||
| done(); | ||
| collection.dropIndex('a_1').then( |
There was a problem hiding this comment.
nit: I'd prefer to use .then/.catch vs this form if possible
There was a problem hiding this comment.
.catch is technically not part of A+, so if someone swaps in another A+ promise library, it could break.
test/functional/session_leak_test.js
Outdated
| const MongoClient = require('../../lib/mongo_client'); | ||
| const ServerSessionPool = core.Sessions.ServerSessionPool; | ||
|
|
||
| (() => { |
There was a problem hiding this comment.
why is this wrapped in an IIFE?
There was a problem hiding this comment.
I don't think it actually have to be. I'll remove it.
There was a problem hiding this comment.
I'm not sure A+ matters anymore now that this is part of the spec. Take a look here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Specifications, everything that supports Promises supports catch
There was a problem hiding this comment.
Ok, I'll change that :)
daprahamian
changed the title
|
@mbroadst This has passed CI, and should be good to merge (though we may want to remove the hardcoded |
… tests Makes sure that sinon doesn't throw errors when tests are skipped
daprahamian
force-pushed
the
HELP-5384
branch
from
cd1f216
to
cfc4ca5
Compare
Part of HELP-5834
Cannot be merged right now, as it needs a new release of mongodb-core