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

Fix bug where domains aren't re-entered when handlers are executed #110

Merged
merged 7 commits into from
Nov 25, 2024

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Nov 22, 2024

Fixes tidyverse/ellmer#163

Intro to promise domains: https://gist.github.com/jcheng5/b1c87bb416f6153643cd0470ac756231

This example has always worked correctly:

library(promises)

dom <- new_promise_domain()
with_promise_domain(dom, {
  promise_resolve(TRUE) %...>% {
    stopifnot(!is.null(promises:::current_promise_domain()))
    message("success")
  }
})

Meaning, inside of a promise fulfilled handler, the promise domain is correctly in place. However, when promise handlers are nested:

library(promises)

dom <- new_promise_domain()
with_promise_domain(dom, {
  promise_resolve(TRUE) %...>% {
    promise_resolve(TRUE) %...>% {
      stopifnot(identical(promises:::current_promise_domain(), dom))
      message("success")
    }
  }
})

then this test would fail. (Side comment: currently these are both failing with the CRAN version and both passing with the PR version, something must be wrong with the way I wrote these examples!?)

Update: Sorry, I see what's wrong with the above tests--it makes sense that they both fail with CRAN. If dom was a real domain that had some kind of side effect (like setting an env var), that side effect would be in place for the first example, but the domain wouldn't be installed as the current promise domain--that was the entire point of the issue that this PR fixes. Let me try again:

library(promises)
library(withr)

envvar_domain <- function(new) {
  promises::new_promise_domain(
    wrapOnFulfilled = function(onFulfilled) {
      function(value, visible) {
        with_envvar(new, onFulfilled(value, visible))
      }
    },
    wrapOnRejected = function(onRejected) {
      function(reason) {
        with_envvar(new, onRejected(reason))
      }
    },
    wrapSync = function(expr) {
      with_envvar(new, expr)
    }
  )
}

# Passes with CRAN
with_promise_domain(envvar_domain(list(FOO="1")), {
  promise_resolve(TRUE) %...>% {
    stopifnot(Sys.getenv("FOO") == "1")
    message("success")
  }
})

# Fails with CRAN, passes with PR
with_promise_domain(envvar_domain(list(FOO="1")), {
  promise_resolve(TRUE) %...>% {
    promise_resolve(TRUE) %...>% {
      stopifnot(Sys.getenv("FOO") == "1")
      message("success")
    }
  }
})

jcheng5 and others added 3 commits November 22, 2024 13:13
It's the one labeled "This tests if promise domain
membership infects subscriptions made in handlers."

The inner handler wasn't ever executing, due to
some kind of circularity I think. Not sure. Now it's
executing but not with the right promise domain
(meaning promise domain reentry isn't happening).
When a handler is subscribed to a promise (using then()
or similar) and a promise domain is active, then when
the handler is invoked, the promise domain should be
made active again. This allows the domain to "infect"
handlers that are registered later, but in the same
scope.
@jcheng5
Copy link
Member Author

jcheng5 commented Nov 22, 2024

WARNING: Some of these async tests can fail and not fail testthat, resulting in blue checks! Example

Previously, some tests were promise-chaining handlers containing
testthat assertions, and then calling wait_for_it(). This would
cause those assertions to be printed but not treated as test
failures, because wait_for_it() doesn't error on unhandled
promise errors. With this change, wait_for_it() takes a promise
and will raise an error if the promise rejects.

The bigger part of the commit is revisiting all the existing
uses of wait_for_it() and passing in promises if appropriate.
@jcheng5 jcheng5 requested a review from schloerke November 25, 2024 20:38
@jcheng5 jcheng5 merged commit 48434c4 into main Nov 25, 2024
31 checks passed
@jcheng5 jcheng5 deleted the domain-reentry branch November 25, 2024 21:12
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.

Tool calls lose async context
2 participants