Delaying for more than ~24.8 days causes TimeoutOverflowWarning and immediate promise resolution

[cdellacqua]

The default implementation of setTimeout can only accept values within the range of positive signed 32-bit integers. Passing a timeout greater than (2^31)-1 milliseconds (0x7FFFFFFF) yields a TimeoutOverflowWarning and causes the runtime to ignore the specified timeout, assuming a value of 1ms (which by the way seems a dangerous default, I would have opted for using the maximum supported value or straight out throw an error).

This unexpected behavior is not documented in the README, but rather than documenting this quirk of the runtime I'd like to propose a solution: instead of relying on the bare setTimeout, delay could use a wrapped version of it. Something like this:

const maxSupportedTimeout = 0x7fffffff;
type TimeoutContext = { id: ReturnType<typeof setTimeout> };
const defaultSetTimeout = (
  callback: (...args: any[]) => void,
  ms: number,
  timeoutContext?: Partial<TimeoutContext>
): TimeoutContext => {
  timeoutContext = timeoutContext || { id: undefined };
  if (ms > maxSupportedTimeout) {
    timeoutContext.id = setTimeout(
      defaultSetTimeout,
      maxSupportedTimeout,
      callback,
      ms - maxSupportedTimeout,
      timeoutContext
    );
  } else {
    timeoutContext.id = setTimeout(callback, ms);
  }
  return timeoutContext as TimeoutContext;
};
const defaultClearTimeout = (timeoutContext: TimeoutContext) =>
  clearTimeout(timeoutContext.id);

The timeoutContext is passed by reference, therefore each time defaultSetTimeout gets called the timeout id gets updated, so that defaultClearTimeout can always access the most recent value.

[sindresorhus]

I think it would be best to just throw a human-friendly error if the value is too large and clearly document the behavior. We can also specially handle Infinity and just not give it a timeout.

[papb]

I think it would be best to just throw a human-friendly error if the value is too large and clearly document the behavior.

Why? Wouldn't supporting any values be simpler for users? This way they wouldn't need to remember that a limit exists.

[sindresorhus]

@papb A delay above 24 days makes no sense though. It's most likely a mistake and it would be better to fail loudly than to silently accept it.

[cdellacqua]

24 days makes no sense

I found out about this bug because I needed a delay of more than 24 days. There definitely are (rare) situations in which it might be needed, and 24 days is quite a random threshold.

I’m not saying I disagree with throwing an error though, I’m quite on the fence on this, but leaning towards accepting any positive integer.

The “support any number” argument is more natural to me because as a user I don’t care about random setTimeout limits which are an implementation detail of the library.

[sindresorhus]

Alright. I'm ok with supporting any number. I also like it when users don't have to care about implementation details. But I think the readme should note that the time will not be accurate because of clock drift and other factors and also that it's based on a suspending clock, not a continuous clock (meaning time will pause if the machine sleeps).