-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
base: production
Are you sure you want to change the base?
Add warning about polluting global scope with bindings derivatives #14508
Conversation
Deploying cloudflare-docs with Cloudflare Pages
|
Files with changes (up to 15)
|
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.
@kodster28 this is good addition from @GregBrimble — is a fun tricky one to explain well. taking a crack but eyes helpful
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. |
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.
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?
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.
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.
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.
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. |
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.
Maybe lead with what you should do, then have the shouldn't do?
Co-authored-by: Brendan Irvine-Broque <birvine-broque@cloudflare.com>
Co-authored-by: Kody Jackson <kody@cloudflare.com>
You can think of a binding as a permission and an API in one piece. With bindings, you never have to add secret keys or tokens to your Worker in order to access resources on your Cloudflare account — the permission is embedded within the API itself. The underlying secret is never exposed to your Worker's code, and therefore can't be accidentally leaked. |
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.
Is this "never have to" actually true? Are there cloudflare services for which there is no binding currently? I just always get nervous when I see absolute statements like this in docs.
|
||
```ts | ||
let client = undefined; |
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.
nit: I would add a comment here like: // DON'T DO THIS
to make it extra clear
``` | ||
|
||
If you have more advanced needs, explore the [AsyncLocalStorage API](/workers/runtime-apis/nodejs/asynclocalstorage/), which provides a mechanism for exposing values down to child execution handlers. |
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.
Hmm.. I need to verify but is it possible that the environment/bindings can be reloading while a request is being processed? If for instance, the request is pending some async i/o, can the bindings be updated on the existing isolate while that i/o is pending? If it can, then even using AsyncLocalStorage
to propagate the client
in this example could potentially be problematic by propagating the binding as it existed before the update! The most reliable approach is likely to just access the binding in exactly the execution scope that it is used.
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.
We've seen people do stuff like:
let thingOnGlobal;
export default {
fetch(request, env, ctx) {
thingOnGlobal ??= Math.random();
env.THING_ON_ENV ??= Math.random();
return Response.json({ env, thingOnGlobal });
}
}
Here, env.THING_ON_ENV
behaves very similarly to thingOnGlobal
. They'll both get initialized with a value when the first request comes in and it'll stay in-memory. The only difference is that env.THING_ON_ENV
will change whenever anything on env
changes, but thingOnGlobal
will only change when the isolate is evicted.
I tried this:
let thingOnGlobal;
export default {
fetch(request, env, ctx) {
thingOnGlobal ??= Math.random();
env.THING_ON_ENV ??= Math.random();
const initial = JSON.stringify({ env, thingOnGlobal });
await new Promise((resolve) => setTimeout(resolve, 10000));
const after = JSON.stringify({ env, thingOnGlobal });
return new Response(`initial: ${initial}\nafter: ${after}`);
}
}
and despite deploying with a change on env
in those 10 seconds, never saw any change to env
's reported value across initial
and after
. I even raced another request in the meantime (which did reflect the new values), and could never get disagreement between initial
and after
.
To me, this makes sense and would have been what I expected. I'd have been surprised if env
did hot-reload with some new values magically. My mental model is that something "calls" my fetch
handler with the request
, env
and ctx
properties as they were at the time of the invocation. I don't personally think of them as hot-reloadable objects — it's just that the next time my handler gets called, they might be passed a new instance of env
(or always a new instance of request
and ctx
).
WorkerEntrypoint
s are maybe a bit different though, since they're only "called" with request
. env
and ctx
exist as props on the class. I tried it out, and got the same behavior as regular handlers, but I think there's an argument that they could be hot-reloadable. We just don't seem to do that today.
No description provided.