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

R callbacks and promises #28

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

jcheng5
Copy link
Contributor

@jcheng5 jcheng5 commented Apr 2, 2024

This is a sketch of how we could safely and efficiently get R callbacks when aio operations complete.

library(nanonext)
library(promises)

socket1 <- socket("req", listen = "inproc://nanonext")
socket2 <- socket("rep", dial = "inproc://nanonext")

recv_aio(socket2) %...>% message("Received message: ", .)

send(socket1, "hello world!")

should print "Received message: hello world!" at the console.

Design

The C functions rnng_recv_aio and rnng_recv_aio_signal take an additional R function argument, which if non-NULL will be invoked on the main R thread at the top of the later event loop.

The R functions recv_aio and recv_aio_signal take advantage of this to support a $callback member on the recvAio environment; if present at the time the request is completed, it will be invoked with the value or error. (If $callback is set after the request is completed, it will never be invoked.)

The as.promise.recvAio uses $callback to implement promise resolution.

A note on API

For this proof of concept, I enhanced both recv_aio and recv_aio_signal to support $callback but have no idea if this is what you'd want to do. As written, every recv_aio operation will cause later to invoke an R callback, even if it's not used. I haven't measured the impact of this performance-wise, but given how lean nanonext runs, it seems like you might want this behavior to be opt-in. Like maybe recv_aio and recv_aio_signal can't be coerced to promises, but a new recv_aio_async returns a promise directly?

Also, I didn't implement this for the send functions but I imagine it would work in much the same way.

@@ -96,6 +98,7 @@ export(until_)
export(wait)
export(wait_)
export(write_cert)
importFrom(later,later)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed just to force the later package to load before nanonext does; there's a R_RegisterCCallable/R_GetCCallable dance that needs to happen, otherwise you get an error when the later_api.h static initializer loads.


if (is.null(prom)) {
prom <- promises::promise(function(resolve, reject) {
assign("callback", function(...) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to use assign here because $<-.recvAio is a no-op. (Took me quite some time to realize that! 😅)

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, sorry about that! :p

#ifndef LATER_SHIM_H
#define LATER_SHIM_H

// This is simply a shim so that later::later can be accessed from C, not C++
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really designed later_api.h for packages that use Rcpp, luckily this little shim is all that's needed to make it work with C. (If you're aware of a better technique than this, please let me know)

Copy link
Owner

Choose a reason for hiding this comment

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

Hi Joe, I'm looking at this and in this commit 68fa503, I switch to using the usual GetCCallable on execLaterNative rather than this shim method. Is there a reason this is to be avoided? I see there is a very old comment block in later here: https://github.com/r-lib/later/blob/main/src/init.c#L57-L74 which says that this interface is to be removed, but I'm not aware of the reason. Thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

I've switched to calling execLaterNative2 in fb14833 which is what later_api.h appears to be doing anyway. It is called once in the package init function, so should be safe as well. Unless there are pitfalls I am not aware of, I prefer this method as it (i) utilises the 'official' R linking API and (ii) does not need a C++ compiler to build the nanonext shared object.

Copy link
Owner

Choose a reason for hiding this comment

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

I am now initializing the GetCCallable to execLaterNative2 when the first promise is 'registered' by calling back into R to load the later namespace. This is inspired by an approach Winston Chang and I took to 'lazy-load' rlang in the later package itself. This means that there is no implementation overhead now for cases that do not use promises.

Comment on lines +206 to +210
PROTECT(callExpr = Rf_lcons(func, R_NilValue)); // Prepare call
PROTECT(result = Rf_eval(callExpr, R_GlobalEnv)); // Execute call

UNPROTECT(2);
R_ReleaseObject(func);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not super confident in this part, co-wrote with ChatGPT. Maybe should be written not to PROTECT the result since we don't use it?

Copy link
Owner

Choose a reason for hiding this comment

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

Time to start using ChatGPT! You're right we don't need to protect the result, or we could even cast the Rf_eval to void. More importantly, we shouldn't be adding/removing potentially large numbers of objects to/from the R precious list - but that's a detail that can be solved.

Copy link
Owner

@shikokuchuo shikokuchuo Apr 11, 2024

Choose a reason for hiding this comment

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

Actually I don't think we need to protect the callback functions in the Precious List, which simplifies things greatly. EDIT: this wasn't the case, but I've made this evaluation safe through R_UnwindProtect. I've continued your work in the 'dev' branch as I don't seem to have permissions to modify this PR. I'm still thinking through what an optimal API would be.

@shikokuchuo
Copy link
Owner

This is simply first rate! Now that I've seen that integration with later works at this level, I am quite confident that this is the way to go.

Most of the issues are just implementation details that can be resolved. However, first I will need to think over some broader issues, such as whether it makes sense to take a dependency on {later} at the {nanonext} level, or whether actually to implement this all at {mirai}. The API point does likely need to be solved in the way you suggest through a dedicated function.

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