fix(NODE-7308): replace process.nextTick with queueMicrotask #4817
Conversation
PavelSafronov
force-pushed
the
NODE-7308
branch
from
5b9abd1 to
52b36e3
Compare
PavelSafronov
force-pushed
the
NODE-7308
branch
from
52b36e3 to
ae2e037
Compare
| // This is a guarantee in node, unless you are performing a transaction (which is not being done in this test) | ||
| expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex')))).to.have.lengthOf(1); | ||
| const uniqueSessionIds = new Set(events.map(ev => ev.command.lsid.id.toString('hex'))); | ||
| expect(uniqueSessionIds).to.have.length.lessThanOrEqual(2); |
There was a problem hiding this comment.
Can you please elaborate on this change?
Maybe some summary from the slack thread would be good for potential future investigations, wdyt?
There was a problem hiding this comment.
Absolutely, adding information about the timing, as well as an example bulkWrite that would have broken the old version of this test.
There was a problem hiding this comment.
Thank you! Sorry for the ambiguity in my original request, I'm not sure if referring "previous version" in the code will help in the future because when you read the code you only see the present version. Maybe it would be enough to provide a context about bulkWrite creates its' own session which leads to the queue being processed in between of checkIn() and endSession(), that results in more than 1 session being created.
And this comment can be left in the PR so the diff is clear.
However, it's just a minor suggestion, I believe what you did works just fine.
There was a problem hiding this comment.
Yup, "previous version" doesn't make too much sense. Here's a different approach:
// We verify that the number of unique session IDs used is less than the number of operations.
//
// The current version of the driver + this test will result in 2 server session being used.
// 1. One server session is used for most operations
// 2. A second server session is created and used for the bulkWrite operation
//
// In the past, we used process.nextTick in our connection checkout logic, and it was possible for bulkWrite to complete its work
// fast enough that it would release its server session and result in only 1 server session being used.
// The current timing behavior results in 2 server sessions being used consistently.
tadjik1
added
the
Primary Review
tadjik1
left a comment
tadjik1
left a comment
There was a problem hiding this comment.
Great work @PavelSafronov, thanks for the investigation around client sessions and queue processing!
3 participants
Description
Summary of Changes
Replace process.nextTick() with .queueMicrotask()
What is the motivation for this change?
This helps us reduce our runtime dependencies, as part of https://jira.mongodb.org/browse/NODE-6601
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript