Node: Unreferenced timer started in uncaughtException handler fires immediately if exception was thrown from inside a timer

[novemberborn]

• Version: v4.8.7, v6.12.3, v8.11.0, v9.11.1
• Platform: MacOS 10.13.4
• Subsystem: Timers

I have an uncaughtException handler which performs asynchronous cleanup. This cleanup may only take a certain amount of time, after which the process is forcibly exited. This time limitation is implemented through an unreferenced setTimeout.

If an uncaught exception is thrown from inside another timer, then my cleanup timeout fires instantly.

Here's a reproduction:

'use strict'

process.on('uncaughtException', err => {
  console.error('caught exception', err)

  let start = Date.now()
  const timer = setTimeout(() => {
    console.error('timeout duration %sms', Date.now() - start)
  }, 1000).unref()
  console.error('created timer')
})

setTimeout(() => {
  throw new Error('trigger')
}, 1000)

setTimeout(() => {}, 5000) // keep process alive

$ node test.js

caught exception Error: trigger
    at Timeout.setTimeout [as _onTimeout] (/private/var/folders/2_/qczp184x76b2nl034sq5hvxw0000gn/T/tmp.iAigvol9mj/test.js:12:9)
    at ontimeout (timers.js:466:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:267:5)
created timer
timeout duration 2ms

The behavior is as expected when the timer is not unreferenced.

[novemberborn]

And yes an uncaught exception occurred so it could be argued the timer behavior is undefined. It is really surprising though.

[apapirovski]

This was fixed at some point recently (works as expected on master) and will be in 10.x but it might've been semver-major. I'll look into which exact commit fixed it.

[apapirovski]

It might've been https://github.com/nodejs/node/pull/18486 which is indeed semver-major.

[novemberborn]

Oh that's great @apapirovski!

It's not clear to me from that PR why it's semver-major?

[apapirovski]

@novemberborn The error handling behaviour is dramatically different now which could break existing code in unexpected ways. I'm not saying it's likely but I think we will want to at least give it some time in that state before we revisit whether it's a good idea to backport and decrease its status from a semver-major change. It's also possible there are new undiscovered bugs so we wouldn't to just land on an LTS release like 8.x.

[apapirovski]

This of course all relies on me being right re: that being the fix. I'm going to do a bit more testing when I have time to confirm.

[Fishrock123]

Sounds like a bug to me.

[apapirovski]

Ok, so it was that PR and the first commit in it that fixed it. I don't think we can backport that fix but I've got a separate one that I'll open a PR for. We should be able to get that into v6.x, v8.x & v9.x, I think.

[apapirovski]

Fix in https://github.com/nodejs/node/pull/20025

[apapirovski]

v8.x fix in https://github.com/nodejs/node/pull/20497

[apapirovski]

Closing now that the fix landed in v8.x-staging