Watchdog depends on libuv loop ordering

[davisjam]

Version: 8.8.1
Platform: Linux jamie-Lenovo-K450e 4.8.0-56-generic #61~16.04.1-Ubuntu SMP Wed Jun 14 11:58:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
Subsystem: node_watchdog.cc (affects the VM module)

Here's code from node_watchdog.cc, which is used in node_contextify.cc to time out runaway executions in a VM context.

Watchdog::~Watchdog() {
  uv_async_send(&async_);
  uv_thread_join(&thread_);
  ...
}

void Watchdog::Async(uv_async_t* async) {
  Watchdog* w = ContainerOf(&Watchdog::async_, async);
  uv_stop(w->loop_);
}

void Watchdog::Timer(uv_timer_t* timer) {
  Watchdog* w = ContainerOf(&Watchdog::timer_, timer);
  *w->timed_out_ = true;
  w->isolate()->TerminateExecution();
  uv_stop(w->loop_);
}

Summary of behavior

1. In the destructor, we uv_async_send to trigger Watchdog::Async.
2. It may be that Watchdog::Async goes off before Watchdog::Timer, in which case we would like the Timer not to do anything -- specifically, it should neither publish timed_out_ nor call TerminateExecution.
3. From the libuv docs (and code), it looks to me like the Timer currently won't be triggered if the Async goes off first and calls uv_stop, but I might be mistaken.
4. Certainly the Async will always go off, so if the Timer goes off first then we will have Timer -> Async.

Suggestion
Whether or not Async -> Timer can currently happen, I think relying on the libuv loop ordering like this is in poor taste. It looks like the idea is for exactly one of Async and Timer to take effect. Adding internal aborted and timed_out flags and setting/testing the appropriate flag in Async and in Timer would give us this guarantee.

PR
I have a patch that generalizes Watchdog to call an aborted_cb and a timed_out_cb and will only call one of them, plus an appropriate modification to node_contextify.cc.

Would either component of this patch be of interest?

[bnoordhuis]

Libuv's event loop phases are well-documented. Relying on their order in the watchdog is fine.

From the libuv docs (and code), it looks to me like the Timer currently won't be triggered if the Async goes off first and calls uv_stop, but I might be mistaken.

No, both can happen. The 'is stopped?' check is done only once per event loop "tick."

It may be that Watchdog::Async goes off before Watchdog::Timer, in which case we would like the Timer not to do anything -- specifically, it should neither publish timed_out_ nor call TerminateExecution.

The scenario you describe only happens when both fire in the same tick. I don't see the current implementation as wrong because the exact timing of the async event is essentially arbitrary.

[davisjam]

Thanks @bnoordhuis.

So If the Async and the Timer can both fire in the same tick, with the Async preceding the Timer, then the Timer will call TerminateExecution even though the script completed. Could this have any ill effect?

[bnoordhuis]

No. uv_stop() gets called twice but it's idempotent.

[davisjam]

OK, I'll close this then. Thanks for your thoughts.