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

Add warning about polluting global scope with bindings derivatives #14508

Open
wants to merge 1 commit into
base: production
Choose a base branch
from

Conversation

GregBrimble
Copy link
Member

No description provided.

Copy link

Deploying cloudflare-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 76432dc
Status: ✅  Deploy successful!
Preview URL: https://e0b5bd7c.cloudflare-docs-7ou.pages.dev
Branch Preview URL: https://add-warning-about-polluting.cloudflare-docs-7ou.pages.dev

View logs

Copy link

Copy link
Contributor

@irvinebroque irvinebroque left a comment

Choose a reason for hiding this comment

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

@kodster28 this is good addition from @GregBrimble — is a fun tricky one to explain well. taking a crack but eyes helpful

## Making changes to bindings

When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.
When you deploy a change to your Worker, and only change its bindings (i.e. you don't change the Worker's code), Cloudflare will reuse existing isolates that are already running your Worker. This improves performance — you can change an environment variable or other binding without unnecessarily reloading your code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.
When a new [version or deployment](/workers/configuration/versions-and-deployments/) only changes bindings - and not the code itself - your Worker generally has better performance. Isolates run with a dynamic set of bindings, meaning they do not need to restart with every change to bindings. So if an isolate is "hot" - or already responding to traffic - the isolate usually remains "hot" after non-code changes.

When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.

However, this does mean that you must be careful when "polluting" global scope with derivatives of your bindings. For example, if you create an external client instance which uses a secret API key on `env`, you must ensure that you don't place this client instance in a global scope. If you do, the client instance might continue to exist despite making changes to the secret which is likely undesirable. Instead of polluting global scope, you should create a new client instance for each request, or, if you have more advanced needs, you may want to explore the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/) which provides another mechanism for exposing values down to child execution handlers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
However, this does mean that you must be careful when "polluting" global scope with derivatives of your bindings. For example, if you create an external client instance which uses a secret API key on `env`, you must ensure that you don't place this client instance in a global scope. If you do, the client instance might continue to exist despite making changes to the secret which is likely undesirable. Instead of polluting global scope, you should create a new client instance for each request, or, if you have more advanced needs, you may want to explore the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/) which provides another mechanism for exposing values down to child execution handlers.
This means you should not assign the values of bindings to variables in global scope. For example, you should not do this, because the client assigned to global scope can continue using the old value of `SECRET_KEY` even after you update it in your Worker:
let client;
export default {
async fetch(request) {
if (!client) { client = new ApiClient(env.SECRET_KEY) }
// ...
}
}
Instead, you should either create a new client instance for each request:
export default {
async fetch(request) {
let client = new ApiClient(env.SECRET_KEY)
// ...
}
}
Or use the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/):
...example

Think clearer with code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even, store the client in a global Map where the key is the env value(s) or hash thereof 🧠

Just on the AsyncLocalStorage note, that's still only going to be per-request, so you'd still be creating "a new client instance for each request". Just would want to make that clear if we were providing code for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe lead with what you should do, then have the shouldn't do?

@irvinebroque irvinebroque requested a review from jasnell May 10, 2024 12:06
## Making changes to bindings

When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.
When a new [version or deployment](/workers/configuration/versions-and-deployments/) only changes bindings - and not the code itself - your Worker generally has better performance. Isolates run with a dynamic set of bindings, meaning they do not need to restart with every change to bindings. So if an isolate is "hot" - or already responding to traffic - the isolate usually remains "hot" after non-code changes.

When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.

However, this does mean that you must be careful when "polluting" global scope with derivatives of your bindings. For example, if you create an external client instance which uses a secret API key on `env`, you must ensure that you don't place this client instance in a global scope. If you do, the client instance might continue to exist despite making changes to the secret which is likely undesirable. Instead of polluting global scope, you should create a new client instance for each request, or, if you have more advanced needs, you may want to explore the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/) which provides another mechanism for exposing values down to child execution handlers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe lead with what you should do, then have the shouldn't do?

When creating a new Worker [version or deployment](/workers/configuration/versions-and-deployments/) and you only include changes to bindings (i.e. you don't change the code of the Worker), be aware that the any currently running Worker isolates may stay "hot" for a potentially long time if they continue to receive traffic. This is an optimization of our platform, allowing Workers to run with a dynamic set of bindings without needing to start a new isolate with every change to bindings. Broadly, this means you can expect better performance when rolling out non-code changes to your Worker.

However, this does mean that you must be careful when "polluting" global scope with derivatives of your bindings. For example, if you create an external client instance which uses a secret API key on `env`, you must ensure that you don't place this client instance in a global scope. If you do, the client instance might continue to exist despite making changes to the secret which is likely undesirable. Instead of polluting global scope, you should create a new client instance for each request, or, if you have more advanced needs, you may want to explore the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/) which provides another mechanism for exposing values down to child execution handlers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
However, this does mean that you must be careful when "polluting" global scope with derivatives of your bindings. For example, if you create an external client instance which uses a secret API key on `env`, you must ensure that you don't place this client instance in a global scope. If you do, the client instance might continue to exist despite making changes to the secret which is likely undesirable. Instead of polluting global scope, you should create a new client instance for each request, or, if you have more advanced needs, you may want to explore the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/) which provides another mechanism for exposing values down to child execution handlers.
At the same time, be careful about which derivatives you add to the global scope. Anything you add might continue to exist despite making changes. For example, an external client instance uses a secret API key on `env`. If you put this client instance in a global scope and also make changes to the secret, the instance might continue to exist. The correct approach would be to create a new client instance for each request. Or - if you have more advanced needs - explore the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/), which provides another mechanism for exposing values down to child execution handlers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product:workers Related to Workers product size/xs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants