IDEA: add a onFinally arg in hold

[cderv]

That would be passed to promises::finally to execute before returning the promise value

promises::then(
    promise,
    onFulfilled = function(value) {
      state$pending <- FALSE
      state$fulfilled <- TRUE
      state$value <- value
    },
    onRejected = function(error) {
      state$pending <- FALSE
      state$fulfilled <- FALSE
      state$reason <- error
    }
  ) %>%
  promises::finally(onFinally = fun)

I have thought about that while working on chrome_execute thinking that chrome could be closed at the very end with

invisible(hold(results_available, timeout = total_timeout, onFinally = ~ chrome$close()))

Thoughts ?

[RLesur]

Very nice idea!
AFAIK, I don't think that this would ensure that chrome$close() would be executed at the very end. If I understand well how promises work, I think that onFinally is executed before onFulfilled and onRejected. I am not 100% sure. This needs some tests.
Just a question: why not using on.exit() in hold()? This would ensure that chrome$close() would be executed at the very end?

[cderv]

If I understand well how promises work, I think that onFinally is executed before onFulfilled and onRejected. I am not 100% sure. This needs some tests.

I was understanding that finally was executed at the very end. This needs some tests to be sure.

why not using ˋon.exit()`

Because in chrome execute you were closing with finally and this is what gave me the idea. ☺️ for closing on.exit would work I think.
But the idea is not just for closing. I was thinking about how this could improve ˋhold`.

But are finally and on.exit the same if finally is the last thing executed ? Seems like it is ... 🤔

[RLesur]

Sorry, I was 100% wrong.
I always struggle with onFinally (TBH, I dislike it). Here is why.
Take this dummy example:

library(promises)

promise <- promise_resolve(1)
then(
  promise,
  onFulfilled = function(value) {
    cat("onfulfilled\n")    
  }
) %>%
  finally(onFinally = ~ stop("something went wrong in finalizer"))

If you run this script, you get an unhandled rejected promise.
This comes from the fact that promises::finally() returns a promise.
So, we have to handle exception for this new promise with a promises::catch().
Since, the goal of hold() is to go out an async process, my fear is that using finally(), we go back in an async process because we create a new promise...

[RLesur]

From the previous example, I think the safest way to use promises::finally() in hold() would be:

promises::then(
  promises::finally(promise, onFinally = fun),
  onFulfilled = function(value) {
    state$pending <- FALSE
    state$fulfilled <- TRUE
    state$value <- value
  },
  onRejected = function(error) {
    state$pending <- FALSE
    state$fulfilled <- FALSE
    state$reason <- error
  }
)

The main difference with on.exit() is about the timeout.
In the above example, the timeout stands for the execution of the promise + the time for the finalizer to execute.
Using on.exit(), the timeout would mean the delay for the promise resolution.
Honestly, I am indifferent.

[cderv]

The second choice is maybe simpler ?

[RLesur]

I agree: saying that the timeout only covers the promise resolution seems more simple.