Implements wait_for()#190
Conversation
|
After putting together this PR, I came across the version of |
|
This looks like a great start @shikokuchuo! Have you reviewed the chromote implementation? Currently this PR resembles various implementations we've used in package testing, which is a much more controlled environment. For a public, robust and final solution in promises we should be very convinced about the features we do or do not need from the chromote implementation. |
|
Thanks @gadenbuie I hadn't actually reviewed chromote as I was under the impression that it wasn't an R-level implementation, let alone one based on promises! Luckily it seems we all converge on the same approach. Chromote additionally has a lot of code to handle interrupts and my gut reaction is that this isn't necessary in the general case, but this should become apparent when I actually write the tests! |
I think this gets at what I meant by the difference between the implementations we've used in testing and a user-facing API. My inclination is that in user-facing code we'd want, or might even need, to support interrupts and other features that users would need and expect in an interactive session. |
|
Yes. Both @gadenbuie and I would like to have the chromote version with interrupt support. Thank you, @shikokuchuo ! |
|
Just so we're on the same page, the The version in chromote seems to be to support the interrupt cancelling the underlying promise itself, and is specifically used together with later_with_interrupt() which is a non-exported function internal to chromote. |
Thanks for clarifying, that's great to hear.
I'm happy to wait to see what you find out when you write the tests, but it'd be helpful in the end if this PR description can include a summary of the chromote code and an explanation of why we don't need to follow the same path here. It'd also be useful to explain whether we can replace chromote's version with promises' solution without breaking backcompat. |
…hings change in future
|
I'm reviving this PR as I'm not seeing anything obviously wrong with it now (having tweaked it to throw the original condition on rejected promise). Perhaps someone else will see it... |
| } | ||
| private <- attr(promise, "promise_impl")$.__enclos_env__$private | ||
| while (private$state == "pending") { | ||
| later::run_now(Inf, loop = loop) |
There was a problem hiding this comment.
This can only consume a single loop at a time, right?
| if (!is.promise(promise)) { | ||
| stop("wait_for() requires a promise object") | ||
| } |
There was a problem hiding this comment.
What about the following:
f <- \() promise_resolve(42)
g <- \() 42
h <- \() ifelse(runif() < 0.5, f(), g())
h() |> wait_for()There was a problem hiding this comment.
Just burned myself with mirai and this recently actually: mirais need to be wrapped with as.promise first. So maybe not a good idea to make this magically work for all types.
Is as.promise designed to be idempotent in general?
| later::run_now(Inf, loop = loop) | ||
| } | ||
| if (private$state == "rejected") { | ||
| stop(private$value) |
4 participants
Closes #142, closes #48.
Implements
wait_for()— a general-purpose function that synchronizes a promise by blocking until it resolves or rejects, then returns the value or raises the original error condition.Interrupt behaviour
Chromote's
wait_for()uses asynchronize()function with an interrupt domain that, on Ctrl+C:interruptedflagtools::pskill()This is necessary because chromote communicates with an external Chrome process over WebSocket — pending CDP commands must be cancelled and the connection cleaned up gracefully. The interrupt domain (~100 lines) is implemented in chromote because it requires application-specific cleanup logic (cancelling CDP commands, closing WebSocket connections). A general-purpose
wait_for()cannot know what cancellation means for an arbitrary promise.Our
wait_for()lets the interrupt propagate naturally. This is the correct approach for a general-purpose utility because most promises don't have a meaningful cancel operation — there is no external actor to notify.cc @schloerke @gadenbuie @cpsievert