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

Ensure that prev_mask is always protected #256

Merged
merged 3 commits into from
Mar 17, 2022

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Mar 17, 2022

Closes tidyverse/dplyr#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.

Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

NEWS.md Outdated Show resolved Hide resolved
Co-authored-by: Lionel Henry <lionel.hry@gmail.com>
@DavisVaughan DavisVaughan merged commit 70d7db9 into main Mar 17, 2022
@DavisVaughan DavisVaughan deleted the fix/prev-mask-protection branch March 17, 2022 17:29
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.

Memory leak(?) in dplyr, magrittr, or one of dplyr's dependencies
2 participants