tidyverse / magrittr

Improve the readability of R code with the pipe
https://magrittr.tidyverse.org
Other
957 stars 157 forks source link

Ensure that `prev_mask` is always protected #256

Closed DavisVaughan closed 2 years ago

DavisVaughan commented 2 years ago

Closes https://github.com/tidyverse/dplyr/issues/6207

The most minimal reprex I could come up with to show this issue was:

library(magrittr)

var <- "x"
data <- data.frame(feature = letters[1:10], imp = 1:10)

gctorture2(5L)

for (kk in 1:100) {
  cat(sprintf("Run #%d\n", kk))

  data.frame(feature = letters[1:10]) %>%
    dplyr::left_join(data, by = "feature") %>%
    dplyr::mutate(!!var := 1.0)
}

cat("Done\n")

Which would often error with:

Error in dplyr::mutate(., `:=`(!!var, 1)) : 
   'rho' must be an environment not promise: detected in C-level eval

This only happened when there were >=2 pipes in a pipe chain, and didn't happen at all when the pipes were removed. We also had to be very aggressive with gctorture2() to get it to show up at all, so it was probably very rare.

It turns out that as we are updating prev_mask and moving from one iteration of the while loop to the next, there is a very short window where prev_mask is unprotected and can be gc'd. On the second iteration of the while loop, right after REPROTECT(mask, mask_pi); the prev_mask becomes unprotected. The first line of r_env_bind_lazy() runs an allocating function, which is when it is possible for prev_mask to become gc'd. You can force this to happen reliably by placing R_gc(); right after the REPROTECT(mask, mask_pi); line and running the reprex above.