Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement autorunning of child loops #119

Merged
merged 40 commits into from
May 1, 2020
Merged

Implement autorunning of child loops #119

merged 40 commits into from
May 1, 2020

Conversation

wch
Copy link
Member

@wch wch commented Feb 6, 2020

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.

@wch wch requested a review from jcheng5 February 6, 2020 21:52
R/later.R Outdated Show resolved Hide resolved
src/callback_registry.cpp Outdated Show resolved Hide resolved
return registries[id];
}

bool remove(int id) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ASSERT_MAIN_THREAD()

@jcheng5
Copy link
Member

jcheng5 commented Mar 31, 2020

  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
Copy link
Member Author

wch commented Apr 3, 2020

I've implemented the changes we discussed.

@jcheng5
Copy link
Member

jcheng5 commented Apr 6, 2020

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
Copy link
Member Author

wch commented Apr 8, 2020

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.

  • If the loop has a parent, it should be destroyed when:
    • There are no R references to the loop, AND the loop is empty
  • If the loop does not have a parent, it should be destroyed when:
    • There are no R references to the loop

@jcheng5
Copy link
Member

jcheng5 commented Apr 14, 2020

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.

@wch wch requested a review from jcheng5 April 22, 2020 13:45
src/later.cpp Show resolved Hide resolved
Copy link
Member

@jcheng5 jcheng5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than comments

@wch wch merged commit 1f7de58 into master May 1, 2020
@wch wch deleted the autorun branch May 1, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants