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

r_bg exit status always 0 #291

Open
r2evans opened this issue Nov 7, 2024 · 3 comments
Open

r_bg exit status always 0 #291

r2evans opened this issue Nov 7, 2024 · 3 comments

Comments

@r2evans
Copy link

r2evans commented Nov 7, 2024

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.

@r2evans r2evans changed the title R exit status always 0 r_bg exit status always 0 Nov 7, 2024
@gaborcsardi
Copy link
Member

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
Copy link
Author

r2evans commented Nov 7, 2024

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
Copy link
Member

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.

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

No branches or pull requests

2 participants