-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-4598)!: close cursor on early loop break #3502
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
Conversation
} | ||
|
||
expect(cursor.closed).to.be.true; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add just a couple more cases / Qs:
- Won't the finally logic always be run at the end of the loop or is it only when there's break? We can add an assertion to the existing tests for the expected "closed" state either way.
- Can we add a test that attempt to keep using the cursor after the
for await
has done the clean up, ex.expect(async () => for await (xx)).to.throw(X)
- We have a gate on calling close only if it has not been called already (using the
closed
flag), I'm not sure it's easily reachable since we have the early return based onclosed=true
. I think it's worth capturing that behavior's significance, does close fail or repeat steps if it's called more than once?- We could use the iterator manually, call
cursor[Symbol.asyncIterator]()
and use.return()
+ sinon to check that .close is only called once - We could double check that close does not repeat steps if called more than once (this may be more trouble than it's worth in this PR, but I think
needsToEmitClosed
based on!this[kClosed]
provides idempotency).
- We could use the iterator manually, call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added assertions to the other tests that checks the cursor is closed on every one once they are exahusted. I've also added a test that asserts the second attempted iteration immediately returns, and also spies on the cursor to assert that close()
is called only once. I think these cover the points you were getting at here but let me know if I missed something.
Description
Ensures cursor is closed when exiting the async generator.
What is changing?
Adds a finally clause in the async generator to close the cursor (if it's not already closed). This handles early exits from
for await of...
loops. A new integration test also is added to check this scenario.Is there new documentation needed for these changes?
None
What is the motivation for this change?
NODE-4598
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript