Support setTimeout with delay=0 called from global scope

[IgorMinar]

Calling setTimeout from the global scope (outside of a request context) is currently not supported by workerd. If a code attempts to call it:

setTimeout(() => {
  console.log("we'll never get here :-/");
}, 0);

an exception is thrown by the runtime:

Uncaught Error: Some functionality, such as asynchronous I/O, timeouts, and generating random values, can only be performed while handling a request.

Existing code and libraries currently rely on calling setTimeout with delayMs set to 0 (or undefined) from global scope to schedule tasks or emulate microtask scheduling. A good example is esbuild-wasm (the browser flavor): https://github.com/evanw/esbuild/blob/d8b028fc62bb6a4115a841b9ba9d0c747a0d54d1/lib/npm/browser.ts#L90

While we don't want code running in our server runtime to schedule tasks during isolate boostrap with a delay, the current behavior causes interop and code portability issues. A reasonable compromise would be to allow setInterval to be invoked from a global scope as long as the delay is undefined or is set to 0ms. This could under the hood be implemented by scheduling the task as a microtask via the queueMicrotask API — the difference between a task and microtask should not be observable to the developer in this limited server-only scenario.

A simple polyfill implementation of this change that can be used by anyone blocked by this issue looks like this:

const originalSetTimeout = globalThis.setTimeout;
const originalClearTimeout = globalThis.clearTimeout;

// hack to minimize possibility of possible timer ID collisions
var setTimeoutCounter = Number.MAX_SAFE_INTEGER - 1_000_000;
var pendingTimers = new Set();

globalThis.setTimeout = function setTimeout(callback, delay, ...args) {
  if (delay > 0) {
    return originalSetTimeout(callback, delay, ...args);
  }

  var timerId = setTimeoutCounter++;
  pendingTimers.add(timerId);

  queueMicrotask(() => {
    if (pendingTimers.has(timerId)) {
      pendingTimers.delete(timerId);
      callback();
    }
  });

  return timerId;
};

globalThis.clearTimeout = function clearTimeout(timerId) {
   if (pendingTimers.has(timerId)) {
      pendingTimers.delete(timerId);
  }
  return originalClearTimeout(timerId);
}

[IgorMinar]

This issue came out of the investigation into https://github.com/cloudflare/workers-sdk/issues/2029 with full repro at https://github.com/IgorMinar/repro-workers-sdk-2029-top-level-await

[jasnell]

Existing code and libraries currently rely on calling setTimeout with delayMs set to 0 (or undefined) from global scope to schedule tasks or emulate microtask scheduling.

hmm... we do support queueMicrotask() which really should be used for this case. Or Promise.resolve().then(...), etc, which really should be used for this case. I know, however, that we're not going to get existing code to change so I'm a bit mixed on this one. Allowing setTimeout with delay 0 might set the wrong expectation for users that timers are supported outside of a request context in general.

[kentonv]

@jasnell I already had this same conversation with Igor. :) The reason the existing code uses setTimeout(0) is to yield to the renderer in the browser, which queueMicrotask() doesn't do. In Workers, of course, there is no renderer to yield to. I think it makes sense to implement this for compatibility's sake -- yes it may cause some confusion but it probably solves more problems than it causes.

[IgorMinar]

yeah. @kentonv and I talked about this on Friday. I came across this issue when I was trying to get esbuild to run on Workers.

I get that my request is not ideal as it could enable poor coding practices, but in this case the risk seems to be minimal and we would measurably increase compatibility of our runtime with existing code written primarily with browser runtime in mind.

Alternatively we could support this behavior via a compatibility flag, as long as we also added a special error message for setTimeout(fn, 0) scenarios that would suggest the particular flag to be enabled. Otherwise root-causing the problem via the generic Uncaught Error: Some functionality, such as asynchronous I/O, timeouts, and generating random values, can only be performed while handling a request. error message is tricky and people would not discover the flag.

[jasnell]

Bleh, yeah I understand. I think we should definitely document queueMicrotask as being the better way. As for yielding to the event loop, that's what the scheduler.yield() API is supposed to allow us to do. I haven't introduced that to the runtime yet tho as it's not that far along in the standards process.

I have had some folks ask if we can consider introducing process.nextTick and serImmediate as part of the Node.js Compat story which would give interesting options here also. None of which I actually like, but that's secondary.

[IgorMinar]

yeah. we should be careful about adding these as they will only confuse people, but I understand why for backwards compatibility supporting these (hopefully behind a compat flag) would enable more existing code to run on our platform.

[kentonv]

I would argue against the compat flag here. Just make it work for everyone. That feels like it'll create much less friction than a compat flag, which people who run into this issue will have to figure out how to apply. We can always go back and add a compat flag later if we decide we should have had one (changing the default as of some date).

[jasnell]

I agree. Compat flag should be unnecessary. If we're going to do it let's just do it.

[IgorMinar]

Sgtm

On Sun, Feb 19, 2023 at 8:38 PM James M Snell @.***> wrote:

I agree. Compat flag should be unnecessary. If we're going to do it let's just do it.

— Reply to this email directly, view it on GitHub https://github.com/cloudflare/workerd/issues/389#issuecomment-1436319696, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUZ2HOHBOHK75AGYDKBYTWYLYK3ANCNFSM6AAAAAAVAAAV4A . You are receiving this because you authored the thread.Message ID: @.***>

[jasnell]

On this, what's the expectation, if any when using setInterval(() => {}, 0)?

[IgorMinar]

I think we should error on that. I can’t think of a valid pattern in existing or new code that would require setInterval(() => {}, 0) to be supported.

On Thu, Feb 23, 2023 at 8:42 AM James M Snell @.***> wrote:

On this, what's the expectation, if any when using setInterval(() => {}, 0)?

— Reply to this email directly, view it on GitHub https://github.com/cloudflare/workerd/issues/389#issuecomment-1442093081, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUZ2HHELUOZD3KMPBAE2LWY6HP3ANCNFSM6AAAAAAVAAAV4A . You are receiving this because you authored the thread.Message ID: @.***>

[jasnell]

I'm fine with throwing on it but it is worth pointing out that it does work in browsers.

[IgorMinar]

Right, that’s true, but if we don’t have know interop use-causes, do we want to allow reoccurring tasks scheduled module initializer (from outside of the request context)? That seems undesirable considering our multi-tenant compute model.

On Thu, Feb 23, 2023 at 3:56 PM James M Snell @.***> wrote:

I'm fine with throwing on it but it is worth pointing out that it does work in browsers.

— Reply to this email directly, view it on GitHub https://github.com/cloudflare/workerd/issues/389#issuecomment-1442589362, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUZ2DFSEJXROILY2GJ4I3WY72LLANCNFSM6AAAAAAVAAAV4A . You are receiving this because you authored the thread.Message ID: @.***>

[jasnell]

Oh, not arguing, just saying that it's worth pointing out (in docs, comments, etc) that we are very intentionally not supporting it.

[IgorMinar]

+1, all this stuff is currently (semi-intentionally) under documented and I wish that was not the case because it’s hard to wrap your head around what to expect from our runtime.

On Thu, Feb 23, 2023 at 4:34 PM James M Snell @.***> wrote:

Oh, not arguing, just saying that it's worth pointing out (in docs, comments, etc) that we are very intentionally not supporting it.

— Reply to this email directly, view it on GitHub https://github.com/cloudflare/workerd/issues/389#issuecomment-1442622889, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUZ2ABWDA7DNVAYFZLMA3WY763HANCNFSM6AAAAAAVAAAV4A . You are receiving this because you authored the thread.Message ID: @.***>