r-lib / later

Schedule an R function or formula to run after a specified period of time.
https://r-lib.github.io/later
Other
137 stars 27 forks source link

Implement autorunning of child loops #119

Closed wch closed 4 years ago

wch commented 4 years ago

This PR makes it so that child loops are run automatically when a parent loop is run.

When running an event loop, this checks if the event loop has any children, and runs them as well. It also modifies nextTimestamp() and due() methods so that, when they are called on a registry (AKA event loop), they recurse into children and find the nearest time stamp, or whether any callbacks are due, respectively.

All of this happens in purely C++ code; if we had to call back into R to check for parent-child relationships each time we did this stuff, it would be expensive.

Update: This previously was implemented with external pointers, but it turns out to be much simpler to pass around loop IDs, like we did previously.

jcheng5 commented 4 years ago
  1. The multiple CallbackRegistry mutexes can't be made safe unless you can guarantee the locks are always acquired in the same order. One way out of this would be to have a single mutex protecting all of later('s data structures).
  2. For the global lock approach, all entry points into later would need to be protected by the global lock; execLaterNative2 and execCallbacks included (though the latter would need to be careful NOT to hold the global lock while executing user-supplied callbacks).
wch commented 4 years ago

I've implemented the changes we discussed.

jcheng5 commented 4 years ago

I don't think the finalizer semantics are correct anymore. The finalizer causes the loop to be immediately destroyed. But I think if work is already scheduled on the loop, and its parent still exists, it shouldn't be destroyed.

callback <- function() {
  message("hello")
}
local({
  loop <- later::create_loop()
  later::later(callback, delay = 2, loop = loop)
})
gc(verbose = FALSE)

I think this example should print "hello" after 2 seconds.

wch commented 4 years ago

I've been thinking about this, and I'm not sure that I agree with the proposed semantics. One way of looking at it is that, if you manually create a loop and do weird stuff with it, you're responsible for keeping the reference to the loop until you're done with it. Anyone who is doing stuff like this should be able to grasp the issue, though we will have to document it.

For example, here's the problematic case:

callback <- function() {
  message("hello")
}

f <- function() {
  loop <- create_loop()
  later(callback, 1, loop = loop)
}
f()
gc()
# Wait for 1 second - nothing happens

In order to fix it, the user can wrap the callback with a function that captures the loop:

g <- function() {
  loop <- create_loop()
  later(function() callback(), 1, loop = loop)
}
g()
gc()
# Wait for 1 second - prints "hello"

Also, in cases like WebSocket and Chromote, if someone rm's the object, they probably want it to stop, and not keep doing stuff.


If you still think that we should keep a loop alive when there are callbacks, here's what I think the logic should look like.

jcheng5 commented 4 years ago

I agree with those bullet points. The "it might or might not still execute depending on when a gc happens to occur" seems like it will be a source of bugs that is both surprising and easy to miss during testing.