Open mmuurr opened 2 years ago
How about having the input handler return a function? The function could return the result if the input was valid, and throw if input was not valid. It would look a little different to have input$my_input()
but it’s a pretty weird scenario anyway IMHO.
@jcheng5 oh that's an interesting idea that I hadn't considered. You're right that it'd be a bit awkward to specify the use of the value as a function, but if that's the simplest solution here I may try it. Thanks for the suggestion!
I do think it might still be useful to register application-wide default condition handlers, but I also think a simple workaround there might be to re-define (wrap) observe
and observeEvent
at the application-level with pre-configured error-handlers, e.g.:
my_observe <- function(x, ...) observe(tryCatch(x, error = my_error_handler), ...)
... this hack doesn't really work when using quoted=TRUE
, but otherwise helps keep code readable by avoiding repeated tryCatch
blocks scattered amongst all observers.
Another example where I've found myself wanting something like this in the past is with throttle()
(and debounce()
and similars). When throttling reactive values, observers are implicitly created, but application authors don't have an easy opportunity to add their own error-handling to those observers, possibly leading to (undesirable) application crashes. I've solved that particular issue with wrappers like:
throttle_safely <- function(r, millis, priority = 100, domain = shiny::getDefaultReactiveDomain()) {
throttled_r <- shiny::throttle(purrr::safely(r), millis, priority, domain)
function() {
res <- throttled_r()
if (is.null(res$error)) res$result else stop(res$error) ## now an app-level observer can catch that error condition
}
}
But when writing such wrappers, my mind often wanders into thinking that being able to register 'default' condition handlers would be useful, or being able to pass condition handlers to throttle()
(and similars) would be neat. Those are pretty abstract ideas, though, as I admittedly haven't put enough thought into describing what an ideal solution might look like, so I'm probably not being very helpful here :-/ ... apologies!
(Thanks again for the input-handler-returns-a-function idea above!)
I experimented a bit with the input handler returning a function and figured I'd share my thoughts. The naïve implementation looks something like so:
registerInputHandler("my.custom.InputBinding", function(data, ...) {
function() {
my_input_parser(data) ## <-- may throw an error
# stop("foo") ## <-- during dev force an error for the sake of testing
}
})
... with example usage like so:
observe({
tryCatch({
do_something_interesting_with(input$my_input()) ## input$my_input is a function, so must call to fetch actual val
}, error = my_error_hander)
})
On the good side, now we can catch errors (and any other conditions) thrown by the my_input_parser()
input handler.
On the "eh, this isn't so great side", now the value returned by the handler's returned function isn't itself cached, so each call to input$my_input()
requires a re-execution. This inefficiency can be alleviated perhaps by memoizing my_input_parser
, but the increased levels of wrapping (and accompanying unintentional obfuscation) once agains seems a bit much when really the only goal is to optionally specify some custom error-handling for package- or app-authored components, applications, etc.
I think for now I'm punting a bit by altering the input handler to never err, but as another example one might want even in such scenarios to be able to specify something like a log statement:
registerInputHandler("my.custom.InputBinding", function(data, session, input_name) {
tryCatch({
my_input_parser(data)
}, error = function(e) {
## Would like to perhaps include a log statement, but
## custom input author/package may not know apriori
## what logging framework the app is using, if any.
logError(sprintf("input handler error for %s: %s", input_name, conditionMessage(e)))
NULL ## swallow error to prevent uncatchable session/app termination.
})
}, force = TRUE)
... but the logging framework would be specified at the app-level, and thus shouldn't be included in the package that distributes the custom input.
Thank you for this discussion! Your insights are much appreciated.
With regards to the input-handler-returning-a-function idea, sorry I didn't go into more detail earlier, I was on my phone. I meant something like this:
registerInputHandler("my.custom.InputBinding", function(data, ...) {
result <- NULL
error <- FALSE
tryCatch({
result <- my_input_parser(data) ## <-- may throw an error
}, error = function(e) {
error <<- TRUE
result <<- e
})
function() {
if (error) {
stop(result)
} else {
result
}
}
})
But now that I'm reading your message again, I don't think an input handler is where we intend you to put any code that could conceivably throw, unless the client is sending values that didn't originally come from your app code (like putting bogus values into Shiny.setInputValue()
from the JS console--in which case, erroring the session out is absolutely the right thing to do). In a case where a well-intentioned user might understandably write some ill-formed data (like giving them a big window to paste JSON into, but they pasted something other than well-formed JSON) I think the actual checking of well-formed-ness really ought to be after you've accessed a string or whatever via input$x
. Wrap that logic in a reactive, and then you have so many more options for how to deal with errors (maybe you want to use shinyvalidate or shinyFeedback, or need/validate, etc.).
Regarding wrapping of observe
/observeEvent
into my_observe
, yes, that's exactly what I imagined people would do--though you're right that it's tough to get the quoted
behavior right (but it's absolutely possible), so in retrospect it wasn't realistic for me to think that.
library(shiny)
my_observe <- function(expr, env = parent.frame(), quoted = FALSE, ...) {
# There's a more modern way to do this using rlang::inject but I
# don't remember right at this moment
installExprFunction(expr, "func", eval.env = env, quoted = quoted)
observe({
tryCatch({
shiny::withLogErrors(
func()
)
}, error = function(e) {
})
}, ...)
}
ui <- "Hello, world"
server <- function(input, output, session) {
my_observe({
stop("error one")
})
my_observe(quote({
stop("error two")
}), quoted = TRUE)
}
shinyApp(ui, server)
Regarding the wisdom of this path in general, as a framework author I really want to discourage folks from thinking "catch all errors so the session doesn't crash". There are far worse things an app can do than crash, like lie to the user (or lie to the database), or crash only after gaining enough distance from the original cause of the problem that it's now impossible to debug. This problem is at its worst when using code that is side-effecty, and observers are literally only used for their side effects, hence the seemingly harsh behavior of ending the session (I did entertain the notion of stopping the app entirely but decided ending the session was a good enough compromise).
I think there are lots of valid reasons for catching errors and displaying them to the user without stopping the session, I just think app authors should be encouraged to reckon with each case individually. To that end, perhaps observe
and observeEvent
should have an option that provides some different presets for error handling, defaulting to "end session" (other possibilities could be "report to user", "log silently", "stop app", take a custom function...)
Thank you for the note on throttle/debounce, it's absolutely a bug that those are allowed to stop the session simply by wrapping a reactive that errors. I will file a PR to have them relay errors instead, so the consumer of the throttled/debounced reactive can decide how to deal with them.
In my Shiny applications, I currently wrap nearly all observer-executed expressions in
tryCatch
blocks to gracefully handle errors that'd otherwise kill either the application or the session. One mechanism to do this app-wide is to define a few standard error handlers globally, then use those as appropriate within in each observer.Recently I built a custom input (using Shiny's
InputBinding
base JS class/object) and found myself incapable of catching errors that might occur in the input's customregisterInputHandler()
with my usual pattern. For example,In this example, I'm struggling to figure how (if possible at all) to catch an error with
my_error_handler
. When the error is thrown in the input handler, it's outside the context of the observer'shandlerExpr
'stryCatch
structure. I thought I had an 'aha' moment when I realized the reactive value was being evaluated in the observer'seventExpr
(also outside thetryCatch
structure) and immediately tried:... but that didn't work either.
I can always wrap in the input handler's expressions in a
tryCatch
block, but the input is meant to be a standalone structure, not tied to a specific application (indeed, the input would be distributed in its own package separate from, but used by, a Shiny application). One could also do some deferred dynamic error-handler registration as part of the input's first use (i.e. building the input handler's expression at runtime, just prior to registration), but this begins to have a 'hacky' codesmell :-/I'm wondering if there's been any thought into how one might register/use handlers for use within the Shiny framework functions (that live outside the application code). In some cases, the Shiny framework code itself should definitely just abort (the app or the session), but there seem to be other cases (like this one), where it'd be useful to let the application (optionally) handle conditions.