Skip to content

Commit

Permalink
Avoid creating objects in .GlobalEnv
Browse files Browse the repository at this point in the history
  • Loading branch information
gaborcsardi committed Jul 15, 2019
1 parent 69609cb commit 9f7665e
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 8 deletions.
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@

# development version

* `r_session` now avoids creating `data` and `env` objects in the global
environment of the subprocess.

# callr 3.3.0

* callr now sets the `.Last.error` variable for every uncaught callr
Expand Down
2 changes: 2 additions & 0 deletions R/hook.R
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ common_hook <- function() {
data <- env$`__callr_data__`
data$pxlib <- data$load_client_lib(data$sofile)
options(error = function() invokeRestart("abort"))
rm(list = c("data", "env"))
}, list("__envfile__" = env_file))
}

Expand All @@ -25,6 +26,7 @@ session_load_hook <- function(user_hook = NULL) {
ehook <- substitute({
data <- as.environment("tools:callr")$`__callr_data__`
data$pxlib$disable_fd_inheritance()
rm(data)
})

hook <- substitute({ c; e }, list(c = chook, e = ehook))
Expand Down
24 changes: 16 additions & 8 deletions R/r-session.R
Original file line number Diff line number Diff line change
Expand Up @@ -521,25 +521,33 @@ rs__status_expr <- function(code, text = "", fd = 3L) {

rs__prehook <- function(stdout, stderr) {
oexpr <- if (!is.null(stdout)) substitute({
env <- as.environment("tools:callr")$`__callr_data__`
env$.__stdout__ <- env$pxlib$set_stdout_file(`__fn__`)
assign(
".__stdout__",
as.environment("tools:callr")$`__callr_data__`$pxlib$
set_stdout_file(`__fn__`),
envir = as.environment("tools:callr")$`__callr_data__`)
}, list(`__fn__` = stdout))
eexpr <- if (!is.null(stderr)) substitute({
env <- as.environment("tools:callr")$`__callr_data__`
env$.__stderr__ <- env$pxlib$set_stderr_file(`__fn__`)
assign(
".__stderr__",
as.environment("tools:callr")$`__callr_data__`$pxlib$
set_stderr_file(`__fn__`),
envir = as.environment("tools:callr")$`__callr_data__`)
}, list(`__fn__` = stderr))

substitute({ o; e }, list(o = oexpr, e = eexpr))
}

rs__posthook <- function(stdout, stderr) {
oexpr <- if (!is.null(stdout)) substitute({
env <- as.environment("tools:callr")$`__callr_data__`
env$pxlib$set_stdout(env$.__stdout__)
as.environment("tools:callr")$`__callr_data__`$
pxlib$set_stdout(as.environment("tools:callr")$`__callr_data__`$
.__stdout__)
})
eexpr <- if (!is.null(stderr)) substitute({
env <- as.environment("tools:callr")$`__callr_data__`
env$pxlib$set_stderr(env$.__stderr__)
as.environment("tools:callr")$`__callr_data__`$
pxlib$set_stderr(as.environment("tools:callr")$`__callr_data__`$
.__stderr__)
})

substitute({ o; e }, list(o = oexpr, e = eexpr))
Expand Down
3 changes: 3 additions & 0 deletions tests/testthat/test-r-session.R
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ test_that("regular use", {

expect_equal(rs$get_state(), "idle")

## Clean session
expect_identical(rs$run(function() ls(.GlobalEnv)), character())

## Start a command
rs$call(function() 42)

Expand Down

3 comments on commit 9f7665e

@jdblischak
Copy link
Contributor

Choose a reason for hiding this comment

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

@gaborcsardi Thanks for fixing this so quickly. What are your plans for the next release to CRAN? Would you consider submitting a quick patch with this and other bug fixes? I ask because my package workflowr is generating spurious warnings for users because it is detecting these objects in the global environment:

workflowr/workflowr#168 (comment)

@gaborcsardi
Copy link
Member Author

Choose a reason for hiding this comment

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

@jdblischak
Copy link
Contributor

Choose a reason for hiding this comment

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

Great. Thanks so much!

Please sign in to comment.