Skip to content
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

feat(NODE-4598)!: close cursor on early loop break #3502

Merged
merged 5 commits into from
Dec 22, 2022
Merged

feat(NODE-4598)!: close cursor on early loop break #3502

merged 5 commits into from
Dec 22, 2022

Conversation

durran
Copy link
Member

@durran durran commented Dec 22, 2022

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

  • 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

@durran durran changed the title feat(NODE-4598): close cursor on early loop break feat(NODE-4598)!: close cursor on early loop break Dec 22, 2022
@durran durran marked this pull request as ready for review December 22, 2022 18:38
}

expect(cursor.closed).to.be.true;
});
Copy link
Contributor

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 on closed=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).

Copy link
Member Author

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.

@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Dec 22, 2022
@durran durran requested a review from nbbeeken December 22, 2022 19:42
@nbbeeken nbbeeken merged commit 30c0aee into main Dec 22, 2022
@nbbeeken nbbeeken deleted the NODE-4598 branch December 22, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants