HenrikBengtsson / future

:rocket: R package: future: Unified Parallel and Distributed Processing in R for Everyone
https://future.futureverse.org
946 stars 82 forks source link

Catching an error in a `tryCatch()` and then signaling it with `message()` still errors #596

Closed DavisVaughan closed 2 years ago

DavisVaughan commented 2 years ago
library(future)

plan(sequential)

x <- "b"

fn <- function(x) {
  tryCatch(
    expr = exp(x),
    error = function(e) {
      message(e)
      NA_real_
    }
  )
}

# message, with value
fn(x)
#> non-numeric argument to mathematical function
#> [1] NA

# error
fut <- future(fn(x))
value(fut)
#> Error in exp(x): non-numeric argument to mathematical function

From https://github.com/DavisVaughan/furrr/issues/212

DavisVaughan commented 2 years ago

Notably, it works if you do message(conditionMessage(e)) rather than message(e)

library(future)
#> Warning: package 'future' was built under R version 4.1.2

plan(sequential)

x <- "b"

fn <- function(x) {
  tryCatch(
    expr = exp(x),
    error = function(e) {
      message(conditionMessage(e))
      NA_real_
    }
  )
}

fut <- future(fn(x))
value(fut)
#> non-numeric argument to mathematical function
#> [1] NA

I think this might have to do with the fact that message(e) will signal e and give it a default handler, but it looks like here you have a separate handler set up for e that captures it.

> message
function (..., domain = NULL, appendLF = TRUE) 
{
    args <- list(...)
    cond <- if (length(args) == 1L && inherits(args[[1L]], "condition")) {
        if (nargs() > 1L) 
            warning("additional arguments ignored in message()")
        args[[1L]]
    }
    else {
        msg <- .makeMessage(..., domain = domain, appendLF = appendLF)
        call <- sys.call()
        simpleMessage(msg, call)
    }
    defaultHandler <- function(c) {
        cat(conditionMessage(c), file = stderr(), sep = "")
    }
    withRestarts({
        signalCondition(cond)
        defaultHandler(cond)
    }, muffleMessage = function() NULL)
    invisible()
}
DavisVaughan commented 2 years ago

I am now somewhat convinced that there isn't much you could do to prevent this. As I mentioned above, message(e) will re-signal e as an "error" condition, and this is exactly what your {future} code is trying to capture and relay for the user.

I think I would just advise (and maybe document?) that you shouldn't try something like this with futures.

FWIW, we have a similar issue with reprex and the {evaluate} package, which also tries to capture and relay error conditions.

HenrikBengtsson commented 2 years ago

Hi, thanks for forwarding. Yes, I discovered this too, and I'm tracking it in Issue #507.

I am now somewhat convinced that there isn't much you could do to prevent this. As I mentioned above, message(e) will re-signal e as an "error" condition, and this is exactly what your {future} code is trying to capture and relay for the user.

I agree, that is what I also concluded - I don't think there's a workaround (except for possibly a run-time mod of message() and warning() via trace() but that's a no-no)

I think I would just advise (and maybe document?) that you shouldn't try something like this with futures.

Good idea. I'll probably stick it into https://future.futureverse.org/articles/future-4-issues.html for now. There are a few other code practices that would help document.

FWIW, we have a similar issue with reprex and the {evaluate} package, which also tries to capture and relay error conditions.

Nice to hear I'm not alone :)

Closing this one in favor of #507