r-lib / callr

Call R from R
https://callr.r-lib.org/
Other
300 stars 38 forks source link

r_bg exit status always 0 #291

Open r2evans opened 6 days ago

r2evans commented 6 days ago

callr::r_bg always returns 0 even on error, despite R's default behavior (from ?q: The default error handler for non-interactive use effectively calls q("no", 1, FALSE) and returns error status 1.)

$ echo 'quux' | R --vanilla --quiet
> quux
Error: object 'quux' not found
Execution halted
$ echo $?
1

With callr though, we always get 0:

proc <- callr::r_bg(function() quux)
proc$read_all_output()
# [1] "WARNING: ignoring environment value of R_HOME\n"
proc$read_all_error()
# [1] "Error in (function ()  : object 'quux' not found\n"
proc$get_exit_status()
# [1] 0

I recognize I can use rscript_process to get the desired effect:

writeLines("quux", "quux.R")
proc <- callr::rscript_process$new(callr::rscript_process_options(script="quux.R"))
proc$read_all_output()
# [1] "WARNING: ignoring environment value of R_HOME\n"
proc$read_all_error()
# [1] "Error: object 'quux' not found\nExecution halted\n"
proc$get_exit_status()
# [1] 1

I suggest would be more consistent if the error= and interrupt= handlers in the wrapper used by callr::r_bg end with q(save="no", status=1).

modified   R/script.R
@@ -36,6 +36,7 @@ make_vanilla_script_expr <- function(expr_file, res, error,
       }

       base::saveRDS(base::list("error", e2, e), file = base::paste0(`__res__`, ".error"))
+      q(save = "no", status = 1)
     }, list(
          `__res__` = res,
          `__traceback__` = getOption("callr.traceback", FALSE)

With that, I see the desired behavior:

devtools::load_all()
proc <- callr::r_bg(function() quux)
proc$read_all_output()
# [1] "WARNING: ignoring environment value of R_HOME\n"
proc$read_all_error()
# [1] ""
proc$get_exit_status()
# [1] 1

Are there other aspects that would need to be addressed as well? (That is, what else could be changed for this to be a more complete solution?) Or is there rationale for having r_bg always return status 0?

I'm running R-4.3.3 and callr_3.7.6 within emacs/ess on ubuntu-24.04.

gaborcsardi commented 6 days ago

We can probably exit with 1, although AFAIR we also want to run the post-hook, if any.

In general, I expected people to use get_result(), which has an informative error if the process failed:

proc <- callr::r_bg(function() quux)
proc$wait()
proc$get_result()
Error:
! in callr subprocess.
Caused by error in `(function () …`:
! object 'quux' not found
Type .Last.error to see the more details.
.Last.error
<callr_error/rlib_error_3_0/rlib_error/error>
Error:
! in callr subprocess.
Caused by error in `(function () …`:
! object 'quux' not found
---
Backtrace:
1. proc$get_result()
2. callr:::rp_get_result(self, private)
3. callr:::get_result(out, private$options)
4. callr:::throw(callr_remote_error(remerr, output), parent = fix_msg(remerr[[3]]))
---
Subprocess backtrace:
1. base::.handleSimpleError(function (e) …
2. global h(simpleError(msg, call))
r2evans commented 6 days ago

I see, which means q(..) at that point is premature, got it. That suggests knowing it erred and running q(..) post-hook is preferred. I'll look at the code later to see if it's obvious to me. The use of $get_result() is also fairly clear, I guess my mind can get stuck in unix-cli where exit-status is often the canonical indicator. Thanks!

gaborcsardi commented 6 days ago

Thanks for taking a look! Maybe your patch is fine, IDK, I only have vague memories that there is a post-hook. I'll take a look as well, soon.