-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: main
Are you sure you want to change the base?
Conversation
@@ -96,6 +98,7 @@ export(until_) | |||
export(wait) | |||
export(wait_) | |||
export(write_cert) | |||
importFrom(later,later) |
There was a problem hiding this comment.
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(...) { |
There was a problem hiding this comment.
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! 😅)
There was a problem hiding this comment.
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++ |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
PROTECT(callExpr = Rf_lcons(func, R_NilValue)); // Prepare call | ||
PROTECT(result = Rf_eval(callExpr, R_GlobalEnv)); // Execute call | ||
|
||
UNPROTECT(2); | ||
R_ReleaseObject(func); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
6883d83
to
2136560
Compare
This is a sketch of how we could safely and efficiently get R callbacks when aio operations complete.
should print "Received message: hello world!" at the console.
Design
The C functions
rnng_recv_aio
andrnng_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 thelater
event loop.The R functions
recv_aio
andrecv_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
andrecv_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 mayberecv_aio
andrecv_aio_signal
can't be coerced to promises, but a newrecv_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.