If a canceled request leaves a Promise hanging, how will you notice?
It's unclear to me how workerd handles canceled tasks. Javascript Promises themselves don't have a concept of canceling as such, maybe they reject the promises for canceled requests? Or maybe they just throw away the tasks, without giving them any notice -- after all, the whole point seems to be to mitigate harm from bugs causing runaway processing like infinite loops. Can someone confirm what actually happens? I'm having trouble triggering this in an observable way without writing a custom HTTP client.
https://github.com/cloudflare/workerd/blob/5d644d0189246a22e...
I also don't see anything in the Cloudflare docs that would say anything about how they terminate requests that are canceled. Since only individual requests get canceled, they have to leave the runtime instance alive, to process any other requests it's serving. `event.waitUntil` you buys you an extra 30 seconds.
Whenever a Worker initiates some sort of external I/O -- such as invoking `fetch()` -- the I/O task is associated with the incoming request that was running at the time. If the incoming request is canceled, then all outgoing I/O tasks associated with it are also canceled. This all happens within the runtime implementation, without actually invoking any JavaScript, and so is not constrained by JavaScript Promise semantics. Technically, what ends up happening from the JS side is that the Promise that `fetch()` returned simply never resolves, because the runtime throws away the Resolver without invoking it. V8 is smart enough that when the Resolver for a Promise is garbage-collected, V8 can also GC all of the downstream continuations that were waiting on it, since they will never be called. So all the state ends up being GC'd nicely without ever running any more code.
Related: The way the runtime knows which request is running is that it always knows what I/O event caused it to start executing JavaScript, and every time it starts executing JavaScript, it always keeps running until either the microtask queue is empty, or the time limit is hit (which aborts the request). All these microtasks are necessarily downstream effects of the original I/O event that started them, and therefore the runtime concludes that they are all executing code on behalf of the request with which that I/O was associated. Any new I/O initiated by these microtasks is associated with the same request.
These assumptions break, though, when requests share Promises via the global scope. If request A awaits a promise, and request B later resolves that promise, the code in request A continues running... but the runtime still thinks it is running on behalf of request B. Any new I/O initiated at this point is accounted to request B. If the code tries to access any pre-existing I/O objects than belong to request A, it'll throw an exception. That implies in particular that it cannot cause Request A to complete and return a response.
"The script will never generate a response" happens when the runtime sees that it has finished executing all the JavaScript microtasks for a particular request, and no I/O operations have been scheduled. In this case, the runtime correctly concludes that there is no possible way for new microtasks to be executed on behalf of this request ever again, because there is nothing that would trigger them. This can be trivially reproduced by doing `await new Promise(() => {})` -- awaiting a promise that never resolves. The confusing this is it can also be reproduced by awaiting a promise which is going to be resolved by some other request. Since, as described above, this scenario will cause the subsequent code to execute on behalf of that other request, and such execution cannot possibly cause the original request to produce a response, the error is correct -- the first request will never produce a response. But, it's quite confusing if you don't realize that cross-request promise sharing is a no-no.
The easiest and best way to avoid it is to never modify anything in the global scope. If a request never modifies global scope, then there's no way another request could get ahold of its objects, particularly its promises.
That said, the situation obviously isn't intuitive and we'd like to improve here. What I'm really hoping is that we can find a way to efficiently run every request in a "fresh state", so requests cannot see each other's effects at all. But, literally creating an isolate per request would be too expensive; we need something a bit more clever than that.
(I'm the tech lead for Workers and this strange behavior is my fault.)
The real cost of this is that I can't both 1) cache parsed state, just a Response that I need to re-parse every request, while 2) having singleflight characteristic where later requests in same worker wait for first one to do the work. (If you relax number 2 here, you can still use a global without Promises.)
> I think that implies the isDone polling loop never completes
If the first request -- the one which initiates the fetch -- ends up being canceled for some reason, then yeah, the later ones will all end up looping forever waiting for a state change that will never happen. :( You might need some additional code to check if the pending fetch was started unreasonably long ago and if so, assume it is dead and have a new request context take over attempting the fetch.
That said, keep in mind that Cloudflare Workers runs many, many copies of your worker around the world, and as a result caching in the global scope may not be very effective. It's better if you can leverage the Cloudflare cache (either through the Cache API or by formulating your fetch() to be cachable) as that will be shared across the colo (or even region, for tiered caching).
Yeah, the fetch should be cacheable, I was personally mostly trying to avoid repeating parse+transform work on the fetch results.
(I'm neither the author of the blog, or active in the linked issue/project, I just found the blog by googling for the error message I got independently.)
Hi, @panva here. I've previously had Erwin effectively propose the existing code (not the one from the linked article, the one that's in the library today https://github.com/panva/jose/blob/v4.13.0/src/jwks/remote.t...) acknowledging that there is an even more unlikely edge case.
Well, someone was bound to hit that second edge case.
But please help me out on this, the code does not await the _pendingFetch promise and neither does it touch any runtime globals.
So what gives? Please reach out on the cloudflare discord PanvA#6705.
The code will usually work, but it's possible that the request which initializes `_pendingFetch` gets canceled before the fetch completes, in which case all other requests will loop forever waiting for it.
9 comments
[ 4.8 ms ] story [ 25.9 ms ] threadIf a canceled request leaves a Promise hanging, how will you notice?
It's unclear to me how workerd handles canceled tasks. Javascript Promises themselves don't have a concept of canceling as such, maybe they reject the promises for canceled requests? Or maybe they just throw away the tasks, without giving them any notice -- after all, the whole point seems to be to mitigate harm from bugs causing runaway processing like infinite loops. Can someone confirm what actually happens? I'm having trouble triggering this in an observable way without writing a custom HTTP client. https://github.com/cloudflare/workerd/blob/5d644d0189246a22e...
https://github.com/cloudflare/workerd/blob/5d644d0189246a22e...
I also don't see anything in the Cloudflare docs that would say anything about how they terminate requests that are canceled. Since only individual requests get canceled, they have to leave the runtime instance alive, to process any other requests it's serving. `event.waitUntil` you buys you an extra 30 seconds.
https://developers.cloudflare.com/workers/platform/limits/#c...
https://developers.cloudflare.com/workers/runtime-apis/fetch...
Related: The way the runtime knows which request is running is that it always knows what I/O event caused it to start executing JavaScript, and every time it starts executing JavaScript, it always keeps running until either the microtask queue is empty, or the time limit is hit (which aborts the request). All these microtasks are necessarily downstream effects of the original I/O event that started them, and therefore the runtime concludes that they are all executing code on behalf of the request with which that I/O was associated. Any new I/O initiated by these microtasks is associated with the same request.
These assumptions break, though, when requests share Promises via the global scope. If request A awaits a promise, and request B later resolves that promise, the code in request A continues running... but the runtime still thinks it is running on behalf of request B. Any new I/O initiated at this point is accounted to request B. If the code tries to access any pre-existing I/O objects than belong to request A, it'll throw an exception. That implies in particular that it cannot cause Request A to complete and return a response.
"The script will never generate a response" happens when the runtime sees that it has finished executing all the JavaScript microtasks for a particular request, and no I/O operations have been scheduled. In this case, the runtime correctly concludes that there is no possible way for new microtasks to be executed on behalf of this request ever again, because there is nothing that would trigger them. This can be trivially reproduced by doing `await new Promise(() => {})` -- awaiting a promise that never resolves. The confusing this is it can also be reproduced by awaiting a promise which is going to be resolved by some other request. Since, as described above, this scenario will cause the subsequent code to execute on behalf of that other request, and such execution cannot possibly cause the original request to produce a response, the error is correct -- the first request will never produce a response. But, it's quite confusing if you don't realize that cross-request promise sharing is a no-no.
The easiest and best way to avoid it is to never modify anything in the global scope. If a request never modifies global scope, then there's no way another request could get ahold of its objects, particularly its promises.
That said, the situation obviously isn't intuitive and we'd like to improve here. What I'm really hoping is that we can find a way to efficiently run every request in a "fresh state", so requests cannot see each other's effects at all. But, literally creating an isolate per request would be too expensive; we need something a bit more clever than that.
(I'm the tech lead for Workers and this strange behavior is my fault.)
So, in a scenario like https://github.com/panva/jose/commit/e44cd18ea4cf8af173f874c..., I think that implies the isDone polling loop never completes, and all future requests hitting reload hang since _pendingFetch is already set?
Context: https://github.com/panva/jose/issues/355#issuecomment-102795...
The real cost of this is that I can't both 1) cache parsed state, just a Response that I need to re-parse every request, while 2) having singleflight characteristic where later requests in same worker wait for first one to do the work. (If you relax number 2 here, you can still use a global without Promises.)
https://pkg.go.dev/golang.org/x/sync/singleflight
If the first request -- the one which initiates the fetch -- ends up being canceled for some reason, then yeah, the later ones will all end up looping forever waiting for a state change that will never happen. :( You might need some additional code to check if the pending fetch was started unreasonably long ago and if so, assume it is dead and have a new request context take over attempting the fetch.
That said, keep in mind that Cloudflare Workers runs many, many copies of your worker around the world, and as a result caching in the global scope may not be very effective. It's better if you can leverage the Cloudflare cache (either through the Cache API or by formulating your fetch() to be cachable) as that will be shared across the colo (or even region, for tiered caching).
(I'm neither the author of the blog, or active in the linked issue/project, I just found the blog by googling for the error message I got independently.)
Well, someone was bound to hit that second edge case.
But please help me out on this, the code does not await the _pendingFetch promise and neither does it touch any runtime globals.
So what gives? Please reach out on the cloudflare discord PanvA#6705.