eventloop: Support adding timers & intervals to event loop from Go

[nwidger]

Support adding timers & intervals to an event loop from Go using two new methods SetTimeout and SetInterval, which return a Timer and Interval, respectively. Timers can be canceled by passing them to the new ClearTimeout method (or the clearTimeout function in JavaScript) and Intervals can be canceled by passing them to the new ClearInterval method (or the clearInterval function in JavaScript).

Note that the Timer and Interval types are now exported to allow passing Timers and Intervals between Go and JavaScript.

The time.Timer field was removed from Timer since it is not needed and in fact causes a race condition. Previously, the (time.Timer).Stop() method was called in clearTimeout() to cancel the pending send on the loop's job channel. However, this creates a possible race condition that can cause the loop's run() loop to hang:

• setTimeout() calls addTimeout() which starts timer and increments job count
• run() sees non-zero job count and blocks on job channel
• clearTimeout() stops timer before job is written to job channel
• run() hangs on job channel receive

Instead, clearTimeout() now simply marks the Timer as canceled, but allows the job to be sent on the job channel when the *time.Timer started by time.AfterFunc() fires. When the job is received by run(), doTimeout() will run but will do nothing since the Timer has been marked as canceled.

Add three tests TestNativeTimeout, TestNativeClearTimeout and TestNativeInterval to verify the functionality of the new methods.

[dop251]

There is no race condition because both setTimeout() and clearTimeout() can only be called from within the loop which means they cannot be called concurrently.

If you want to call it from outside the loop, it would be better to use RunOnLoop(), something like:

func (loop *EventLoop) SetTimeout(fn func(*goja.Runtime), timeout time.Duration) *Timer {
        ch := make(chan *timer)
        loop.RunOnLoop(func(vm *goja.Runtime) {
            ch <- loop.addTimeout(func() { fn(vm) }, timeout)
        })
    return <- ch
}

Clearing should be done the same way, but keep in mind that because it's called outside the loop it may still trigger, so probably makes sense to return a bool like 'time.Timer.Stop()' does.

[nwidger]

@dop251 I think there are problems with using RunOnLoop to implemented SetTimeout. If you call SetTimeout within the event loop, it will hang because the job channel is unbuffered. If you call it outside of the event loop, SetTimeout will block until RunOnLoop is able to send on the job channel, again because the job channel is unbuffered, for potentially a long time. I believe SetTimeout should be non-blocking and return immediately, like its JavaScript counterpart.

[nwidger]

@dop251 Maybe it would help to see an example of the sort of thing I am hoping to be able to do. I want to write native modules that provide promise-based functions which perform some asynchronous operation. Here's an example of a native module that provides a simple http.get(url, timeout) function which returns a promise that resolves to undefined or the body of the response:

https://play.golang.org/p/RdszUDBQUjS

You'll need to run it locally using the version of the eventloop package from this PR.

[dop251]

Your implementation of SetTimeout() and SetInterval() would work, however ClearInterval() will cause a race condition if called outside of the loop. If you don't want it to be synchronous it's pretty easy to fix. If not, however, it's quite tricky to make it work both inside and outside of the loop.

[nwidger]

@dop251 I agree, the current code is not at all threadsafe. I intend to address that before anything might get merged, but I wanted to see what you thought of the basic proposal first. I would really like SetTimeout, SetInterval, ClearTimeout and ClearInterval to all be usable and threadsafe both inside and outside of the event loop, but I agree that will be tricky. I believe any solution will involve extra synchronization overhead in terms of mutex's, buffered channels, etc. Are you opposed to such changes to make EventLoop safe to use across goroutines?

[dop251]

I would definitely like to keep it simple and avoid using locks. The race condition in ClearInterval could be fixed by simply running go loop.RunOnLoop(...). If that works for you I could merge it

[nwidger]

@dop251 If I'm interpreting your suggestion right, ClearInterval would look like this?

func (loop *EventLoop) ClearInterval(i *Interval) {
    go loop.RunOnLoop(func(_ *goja.Runtime) {
        loop.clearInterval(i)
    })
}

but that would mean that the Interval will not have actually been canceled when ClearInterval returns. If an Interval is canceled long enough after it was created in the same event loop job, the Interval's run() goroutine will have already submitted a job to run doInterval, which will run before the job to call clearInterval is submitted by ClearInterval:

func TestNativeClearInterval(t *testing.T) {
    count := 0
    loop := NewEventLoop()
    loop.Run(func(_ *goja.Runtime) {
        i := loop.SetInterval(func(_ *goja.Runtime) {
            t.Log("tick")
            count++
        }, 500*time.Millisecond)
        <-time.After(2 * time.Second)
        loop.ClearInterval(i)
    })
    if count != 0 {
        t.Fatal("Expected interval to fire 0 times, got", count)
    }
}

The call to time.After could be replaced with any blocking operation that takes >500ms. On my machine using the above version of ClearInterval, sometimes the test hangs and other times it fails:

$ go test -v -run TestNativeClearInterval
=== RUN   TestNativeClearInterval
    TestNativeClearInterval: eventloop_test.go:192: tick
    TestNativeClearInterval: eventloop_test.go:192: tick
    TestNativeClearInterval: eventloop_test.go:199: Expected interval to fire 0 times, got 2
--- FAIL: TestNativeClearInterval (2.00s)
FAIL
exit status 1
FAIL    github.com/dop251/goja_nodejs/eventloop 2.004s

I believed that this behavior is different than if we were to call clearInterval directly from JavaScript. However, when I tried to verify this, I found another issue:

func TestClearInterval(t *testing.T) {
    const SCRIPT = `
    var count = 0;
    console.log("calling setInterval");
    var t = setInterval(function() {
        console.log("tick");
    }, 500);
    console.log("calling sleep");
        sleep(2000);
    console.log("calling clearInterval");
        clearInterval(t);
    `

    loop := NewEventLoop()
    prg, err := goja.Compile("main.js", SCRIPT, false)
    if err != nil {
        t.Fatal(err)
    }
    var count int64
    loop.Run(func(vm *goja.Runtime) {
        vm.Set("sleep", func(ms int) {
            <-time.After(time.Duration(ms) * time.Millisecond)
        })
        vm.RunProgram(prg)
        count = vm.Get("count").ToInteger()
    })

    if count != 0 {
        t.Fatal("Expected count 0, got", 0)
    }
}

where the call to sleep() could again be replaced by any sufficiently long-running >500ms operation. The test hangs because clearInterval() blocks trying to send on the Interval's stopChan, but the Interval's run() goroutine is blocked trying to send the doInterval job on the loop's job channel. I was able to avoid the hang by having Interval's stopChan be created with a buffer of 1, however I don't know if that would be an acceptable fix.

[dop251]

I've just pushed a fix that should help.

As for your original question, yes, this way ClearInterval() becomes asynchronous. Otherwise I can't really think of a way to make it work from both inside and outside of the loop without doing horrible dirty hack like this one: https://blog.sgmansfield.com/2015/12/goroutine-ids/ I guess the only reasonable option would be having two distinct functions: ClearInterval() and ClearIntervalInLoop() and make sure it's explained in the documentation...

[nwidger]

@dop251 I just pushed a new version of my PR with my attempt at solving the problems we discussed. There are a lot of changes, but I think they are for the better. I put a fair amount of detail in my commit message, so I'll copy that here. Let me know what you think.

eventloop: Support adding timers & intervals to event loop from Go

Support adding timers & intervals to an event loop from Go using two
new methods SetTimeout and SetInterval, which return a Timer and
Interval, respectively.  Timers can be canceled by passing them to the
new ClearTimeout method or the existing clearTimeout JavaScript
function, and Intervals can be canceled by passing them to the new
ClearInterval method or the existing clearInterval JavaScript
function.  All four new methods SetTimeout, SetInterval, ClearTimeout
and ClearInterval can be called inside or outside of the event loop.

The Timer and Interval types are now exported to allow passing Timers
and Intervals between Go and JavaScript.

The event loop now stores jobs in a priority queue implemented using
the stdlib container/heap package.  The priority queue is sorted first
by earliest execution time and then by earliest added time.  All
access to the priority queue is protected with a mutex.  After
executing the last job, the event loop finds the "nearest" job in the
priority queue (i.e. the job that will execute the soonest) and sleeps
for the remaining time before waking up and executing the job.
Interval jobs are re-added to the priority queue by the event loop
with its new execution time just before executing its current
iteration.

Jobs that are added outside of the loop use a buffered "kick" channel
after the job has been added to the priority queue to wake up the
event loop, thus causing it to recalculate the nearest job and sleep
time.  Kicks sent via the "kick" channel are done in a separate
goroutine so as to not cause the caller to block.  The RunOnLoop,
SetTimeout and SetInterval methods use the "kick" channel as they may
be called outside of the run loop, whereas the setTimeout and
setInterval JavaScript functions do not as they are always executed
within the loop.  Since multiple kicks could be buffered before the
event loop wakes up, the event loop drains the "kick" channel after
receiving a kick to avoid spurious wake-ups causing the nearest job to
be recalculated needlessly.

Add new tests TestNativeTimeout, TestNativeClearTimeout,
TestNativeInterval, TestNativeClearInterval and TestClearInterval to
verify the new functionality.

[dop251]

I think it's probably a bit more complicated than it needs to be. Besides, there is at least one data race on job.cancelled, so you'd need to add even more locks.

Here is my take: https://github.com/dop251/goja_nodejs/commit/773713e2664f3bdd33499faad2ede6489cfbdc1d

Let me know if it works for you.

[nwidger]

@dop251 I puzzled over whether setting job.cancelled true without any locks was thread-safe. I came down on the side that it was safe since run() only ever reads job.cancelled and job.cancelled is only ever set to true in [Cc]learTimeout/[Cc]learInterval. It's possible to try to cancel a timer and for it to happen to late, but I believe that will always be the case when called outside of the event loop/in another goroutine. I also saw no races reported by go test -race, although I'm not sure how definitive that result is. I'm curious if you agree or if I'm missing something?

I'll take a look at your commit and let you know. Thanks!

[nwidger]

@dop251 Your take looks great to me! Do you think the documentation for RunOnLoop, SetTimeout, SetInterval, ClearTimeout and ClearInterval should be updated to indicate that they are safe to be called both inside and outside of the loop?

[nwidger]

@dop251 Since your version introduces the concept of immediate jobs, should we add setIntermediate/clearIntermediate Go methods and JavaScript functions while we're at it?

https://nodejs.org/api/timers.html#timers_setimmediate_callback_args https://nodejs.org/api/timers.html#timers_clearimmediate_immediate

or perhaps that is left for another PR.

[dop251]

I'll add documentation. As for the immediate tasks, quite frankly I forgot about this nodejs feature and it's not going to work for it as the semantics is different. The specs state that "If an immediate timer is queued from inside an executing callback, that timer will not be triggered until the next event loop iteration.", but it's not how it's done and it can't be done this way.

I will call it differently otherwise it gets mighty confusing...

[nwidger]

@dop251 Fine by me, I don't have any need for setIntermediate/clearIntermediate and am no expert on their semantics, but I agree that currently the naming could be misleading.

[nwidger]

@dop251 Thanks for adding this feature!