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-5614): Add support for explicit resource management #4177

Merged
merged 18 commits into from
Aug 7, 2024

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Jul 16, 2024

Description

What is changing?

This PR adds support for explicit resource management to our async disposable resources as outlined in the design.

Beta Namespace

A new entrypoint for the driver has been added, mongodb/lib/beta. This namespace will contain any experimental features our APIs we want to expose to users.

API extractor natively supports tagging things as beta and producing a beta namespace, so this PR configures API extractor to emit a beta namespace for us.

Users can access the beta namespace by importing from mongodb/lib/beta.

Symbol.asyncDispose methods

Symbol.asyncDispose methods have been added to the MongoClient, ClientSession, Cursors and ChangeStream classes. These methods are conditionally defined only when Symbol has an asyncDispose property (i.e., the user has polyfilled this property themselves or they're running on a compatible version of Node.js where it's available).

Our readable streams (streaming cursors and download streams) are also disposable on compatible versions of Node.js because they inherit Symbol.asyncDispose from Node.js.Readable.

Additionally, a method is provided to users to allow them to attach the Symbol.asyncDispose methods to each class manually, if desired. This avoids complications for users when the user is polyfilling Symbol.asyncDispose but they may import the driver (and load its classes) before the polyfill runs. In this case, since Symbol.asyncDispose is not defined when evaluating the driver's source code, no disposal methods are added. The configureExplicitResourceManagement can be called manually to add the disposal methods to each class in this scenario.

Testing

There are three major CI and testing changes:

  1. A new manual test has been added that tests whether or not Symbol.asyncDispose is added to our resources in certain Node.js environments. This test suite also asserts on the expected behavior of asyncDispose - namely, that each implementation calls the correct cleanup method for the resource.
  2. A new test suite has been added that smoke tests driver resource to confirm that they can actually be used with resource management. This is a new subpackage in our testing directory because it requires tslib and additional TS configuration. This test suite confirms that our resources can be used with await using and that they are cleaned up appropriately.
  3. (optional) I enhanced the TS testing matrix a bit during development and decided to leave it in the PR. The changes here primarily test with different versions of Typescript and @types/node. The versions I chose are based on what currently pass to more explicitly test our compatibility. I can revert these changes if desired.

Release Highlight

Support for explicit resource management

The driver now natively supports explicit resource management for MongoClient, ClientSession, ChangeStreams and cursors. Additionally, on compatible Node.js versions, explicit resource management can be used with cursor.stream() and the GridFSDownloadStream, since these classes inherit resource management from Node.js' readable streams.

This feature is experimental and subject to changes at any time. This feature will remain experimental until the proposal has reached stage 4 and Node.js declares its implementation of async disposable resources as stable.

To use explicit resource management with the Node driver, you must:

  • Use Typescript 5.2 or greater (or another bundler that supports resource management)
  • Enable tslib polyfills for your application
  • Either use a compatible Node.js version or polyfill Symbol.asyncDispose (see the TS 5.2 release announcement for more information).

Explicit resource management is a feature that ensures that resources' disposal methods are always called when the resources' scope is exited. For driver resources, explicit resource management guarantees that the resources' corresponding close method is called when the resource goes out of scope.

// before:
{
  try {
    const client = MongoClient.connect('<uri>');
    try {
      const session = client.startSession();
      const cursor = client.db('my-db').collection("my-collection").find({}, { session });
      try {
        const doc = await cursor.next();
      } finally {
        await cursor.close();
      }
    } finally {
      await session.endSession();
    }
  } finally {
    await client.close();
  }
}

// with explicit resource management:
{
  await using client = MongoClient.connect(<uri>);

  await using session = client.startSession();
  await using cursor = client.db('my-db').collection('my-collection').find({}, { session });

  const doc = await cursor.next();
}
// outside of scope, the cursor, session and mongo client will be cleaned up automatically.

The full explicit resource management proposal can be found here.

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

@baileympearson baileympearson changed the title initial commit explicit resource management POC Jul 16, 2024
@baileympearson baileympearson changed the title explicit resource management POC feat(NODE-5614): Add support for explicit resource management Jul 24, 2024
@baileympearson baileympearson marked this pull request as ready for review July 25, 2024 17:22
@nbbeeken nbbeeken self-assigned this Jul 25, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 25, 2024
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much good to go! two small functional changes and some api-extractor fixes.

src/resource_management.ts Outdated Show resolved Hide resolved
src/resource_management.ts Outdated Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
test/explicit-resource-management/package.json Outdated Show resolved Hide resolved
src/resource_management.ts Outdated Show resolved Hide resolved
src/resource_management.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/beta.ts Show resolved Hide resolved
src/resource_management.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/beta.ts Show resolved Hide resolved
src/resource_management.ts Outdated Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
test/explicit-resource-management/package.json Outdated Show resolved Hide resolved
nbbeeken
nbbeeken previously approved these changes Jul 26, 2024
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 26, 2024
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some meta comments ahead of code review proper

test/explicit-resource-management/package.json Outdated Show resolved Hide resolved
.evergreen/config.in.yml Outdated Show resolved Hide resolved
api-extractor.json Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
test/manual/resource_management.test.ts Outdated Show resolved Hide resolved
test/manual/resource_management.test.ts Outdated Show resolved Hide resolved
- add testing docs for the smoke tests
- smoke tests depend on a stable package filename, instead of a filename that
  includes the version
.evergreen/run-resource-management-smoke-tests.sh Outdated Show resolved Hide resolved
test/readme.md Outdated Show resolved Hide resolved
test/readme.md Outdated Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
test/manual/resource_management.test.ts Outdated Show resolved Hide resolved
test/readme.md Outdated Show resolved Hide resolved
api-extractor.json Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left small comment. We can wrap this conversation up on slack, too, if you want to speed up the process!

src/beta.ts Outdated Show resolved Hide resolved
test/explicit-resource-management/main.test.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken merged commit b3f3987 into main Aug 7, 2024
27 of 30 checks passed
@nbbeeken nbbeeken deleted the beta-in-package-poc branch August 7, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
5 participants