diff --git a/DESCRIPTION b/DESCRIPTION index 1af212e6..c8c3d7ae 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -37,7 +37,7 @@ Suggests: jose, jsonlite, knitr, - later, + later (>= 1.3.2.9001), paws.common, promises, rmarkdown, @@ -54,3 +54,4 @@ Config/testthat/start-first: resp-stream, req-perform Encoding: UTF-8 Roxygen: list(markdown = TRUE) RoxygenNote: 7.3.2 +Remotes: r-lib/later diff --git a/NEWS.md b/NEWS.md index 5828a0c3..95049b40 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # httr2 (development version) +* `req_perform_promise()` upgraded to use event-driven async based on waiting efficiently on curl socket activity (#579). + # httr2 1.0.6 * Fix stochastic test failure, particularly on CRAN (#572) diff --git a/R/req-promise.R b/R/req-promise.R index d05eaa41..f157e134 100644 --- a/R/req-promise.R +++ b/R/req-promise.R @@ -9,6 +9,10 @@ #' [promises package documentation](https://rstudio.github.io/promises/articles/promises_01_motivation.html) #' for more details on how to work with the resulting promise object. #' +#' If using together with [later::with_temp_loop()] or other private event loops, +#' a new curl pool made by [curl::new_pool()] should be created for requests made +#' within the loop to ensure that only these requests are being polled by the loop. +#' #' Like with [req_perform_parallel()], exercise caution when using this function; #' it's easy to pummel a server with many simultaneous requests. Also, not all servers #' can handle more than 1 request at a time, so the responses may still return @@ -69,6 +73,15 @@ req_perform_promise <- function(req, check_installed(c("promises", "later")) check_string(path, allow_null = TRUE) + if (missing(pool)) { + if (!identical(later::current_loop(), later::global_loop())) { + cli::cli_abort(c( + "Must supply {.arg pool} when calling {.code later::with_temp_loop()}.", + i = "Do you need {.code pool = curl::new_pool()}?" + )) + } + } + promises::promise( function(resolve, reject) { perf <- PerformancePromise$new( @@ -131,12 +144,19 @@ ensure_pool_poller <- function(pool, reject) { monitor <- pool_poller_monitor(pool) if (monitor$already_going()) return() - poll_pool <- function() { + poll_pool <- function(ready) { tryCatch( { status <- curl::multi_run(0, pool = pool) if (status$pending > 0) { - later::later(poll_pool, delay = 0.1, loop = later::global_loop()) + fds <- curl::multi_fdset(pool = pool) + later::later_fd( + func = poll_pool, + readfds = fds$reads, + writefds = fds$writes, + exceptfds = fds$exceptions, + timeout = fds$timeout + ) } else { monitor$ending() } diff --git a/man/req_perform_promise.Rd b/man/req_perform_promise.Rd index 31b281ac..0573a6d5 100644 --- a/man/req_perform_promise.Rd +++ b/man/req_perform_promise.Rd @@ -29,6 +29,10 @@ is finished. See the \href{https://rstudio.github.io/promises/articles/promises_01_motivation.html}{promises package documentation} for more details on how to work with the resulting promise object. +If using together with \code{\link[later:create_loop]{later::with_temp_loop()}} or other private event loops, +a new curl pool made by \code{\link[curl:multi]{curl::new_pool()}} should be created for requests made +within the loop to ensure that only these requests are being polled by the loop. + Like with \code{\link[=req_perform_parallel]{req_perform_parallel()}}, exercise caution when using this function; it's easy to pummel a server with many simultaneous requests. Also, not all servers can handle more than 1 request at a time, so the responses may still return diff --git a/tests/testthat/_snaps/req-promise.md b/tests/testthat/_snaps/req-promise.md new file mode 100644 index 00000000..d42917ee --- /dev/null +++ b/tests/testthat/_snaps/req-promise.md @@ -0,0 +1,9 @@ +# req_perform_promise uses the default loop + + Code + p4 <- req_perform_promise(request_test("/get")) + Condition + Error in `req_perform_promise()`: + ! Must supply `pool` when calling `later::with_temp_loop()`. + i Do you need `pool = curl::new_pool()`? + diff --git a/tests/testthat/test-req-promise.R b/tests/testthat/test-req-promise.R index 04b2d7bc..841d9c9b 100644 --- a/tests/testthat/test-req-promise.R +++ b/tests/testthat/test-req-promise.R @@ -110,3 +110,39 @@ test_that("req_perform_promise can use non-default pool", { p2_value <- extract_promise(p2) expect_equal(resp_status(p2_value), 200) }) + +test_that("req_perform_promise uses the default loop", { + # The main reason for temp loops is to allow an asynchronous operation to be + # created, waited on, and resolved/rejected inside of a synchronous function, + # all without affecting any asynchronous operations that existed before the + # temp loop was created. + + # This can't proceed within the temp loop + p1 <- req_perform_promise(request_test("/get")) + + later::with_temp_loop({ + # You can create an async response with explicit pool=NULL, but it can't + # proceed until the temp loop is over + p2 <- req_perform_promise(request_test("/get"), pool = NULL) + + # You can create an async response with explicit pool=pool, and it can + # proceed as long as that pool was first used inside of the temp loop + p3 <- req_perform_promise(request_test("/get"), pool = curl::new_pool()) + + # You can't create an async response in the temp loop without explicitly + # specifying a pool + expect_snapshot(p4 <- req_perform_promise(request_test("/get")), error = TRUE) + + # Like I said, you can create this, but it won't work until we get back + # outside the temp loop + expect_null(extract_promise(p2, timeout = 1)) + + # This works fine inside the temp loop, because its pool was first used + # inside + expect_equal(resp_status(extract_promise(p3, timeout = 1)), 200) + }) + + # These work fine now that we're back outside the temp loop + expect_equal(resp_status(extract_promise(p1, timeout = 1)), 200) + expect_equal(resp_status(extract_promise(p2, timeout = 1)), 200) +})