rstudio / shiny

Easy interactive web applications with R
http://shiny.rstudio.com
Other
5.32k stars 1.87k forks source link

graceful error handling in place of shutting down #2904

Closed D3SL closed 4 years ago

D3SL commented 4 years ago

The current behavior of Shiny is (intentionally if I understand right) to completely shut down in a lot of situations where it runs into an error.

For both development and the "classic" use case of interactive visualizations this isn't terrible since nothing's really lost. Especially if it's primarily being used by people already familiar with R.

But imho Shiny (and r for that matter) have grown far beyond that niche and with packages like shinydashboard, the myriad of excel-alikes, and more creating a viable platform for internal tools that democratize access to R's functionality.

I think Shiny's grown enough that it's warranted to consider baking in a more graceful means of error handling, either as a new default or as a global option of "production" vs "developent" mode. At minimum I think an app set to production mode should notify the user of an error with a copy/pasteable log output and at least stay open so they can save their work or continue using any functionality not affected by the error.

wch commented 4 years ago

The stopping on error only occurs if the error bubbles up to an observer, as in observe(...). If an error bubbles up to an output (as in output$x <- renderText(...)), that error is caught, and the message is sent to the browser, which then displays it.

For example, this app will stop:

shinyApp(
  fluidPage(verbatimTextOutput("txt")), 
  function(input, output, server) {
    r <- reactive(stop("foo"))
    observe({
      r()
    })
  }
)

But this app will keep running, and display an error message in the web browser:

library(shiny)
shinyApp(
  fluidPage(verbatimTextOutput("txt")), 
  function(input, output, server) {
    r <- reactive(stop("foo"))
    output$txt <- renderText({
      r()
    })
  }
)

The reason that Shiny behaves this way is because, if an error happens and ends up going to an output, someone will find out about the error. But if an error happens and it ends up at an observer, it could go unnoticed -- the application could silently behave incorrectly, and keep doing that without alerting the user. The idea is that, in production, you really don't want to have unexpected (and silent, from the user's pespective) errors.

If you're doing something in an observer that could cause errors, the way to deal with it is with a try or tryCatch.

D3SL commented 4 years ago

Yes, I am aware of that, I even linked directly to a thread on the RStudio site where that was discussed and an RStudio employee suggested opening a github issue about it and suggesting more robust error handling be added. Which is why it comes across as more than a little patronizing that you would repost something you know I know and even linked to.

My entire point is that behavior is not and honestly probably never should have been acceptable. Every software product has the potential for silent errors. The answer is not crashing to desktop. A crash is a bug, not a feature. Error handling is the sort of thing you work on in alpha testing.

Imagine you're working in Excel and if you don't wrap a multi-line function around each and every cell it will crash to desktop and lose the file you were working on if you make a formula error. Or MSWord closes without saving every time you make a typo. Gmail logs out and wipes your inbox if you mistype someone's email address. Windows gives a Blue Screen of Death every time it has an error instead of a popup.

Instead of telling people to hand code a trycatch into each and every single observer it's time to build in error handling.

Or to put it another way: "If you're doing something that could cause a crash to desktop every time the breeze changes, the way to deal with it is to fix your code instead of telling your users to hold their breath the whole time."

michaelhogersnplm commented 4 years ago

To add to this discussion, for large applications it becomes difficult to prevent all errors even with defensive programming. I understand @wch's point, as you would not want errors to go unnoticed but the current way of handling errors can make for Shiny to feel fragile.

Some personal experience with this. We rewrote a function of the Shiny library such that observers have a built in tryCatch to prevent users from seeing a crashed application. We balance this by logging errors via that tryCatch and using no such tryCatch handling while working in an dev environment. In my view it would be nice if there is a global setting to turn on tryCatch for all observers and an argument that allows to turn off the tryCatch functionality (or vice versa).

wch commented 4 years ago

My intent wasn't to be patronizing. From what you wrote: "The current behavior of Shiny is (intentionally if I understand right) to completely shut down in a lot of situations where it runs into an error," it seemed to me that you were saying that Shiny applications stop when they run into an error. Hence the explanation of the difference between observers and outputs; the latter have built-in exception handlers that relay information to the user.

If you want to draw comparisons, it makes sense to look at other languages and platforms that are used for servers. C++, Java, and Node.js, are commonly used for production web servers, and they all stop on uncaught exceptions. They're actually less forgiving than Shiny, since an uncaught exception on these platforms will cause the process to exit.

The broader point I want to make is that Shiny's behavior with respect to errors was a considered decision. It may not be the one you would have made, but that does not mean that it was a mistake.

wch commented 4 years ago

@michaelhogersnplm You might already have done something like this, but one possible way of having something similar to a global switch is to define a version of observe that's local to the app.

catch_and_log_observer_errors <- TRUE

if (isTRUE(catch_and_log_observer_errors)) {
  observe <- function(x, ...) {
    x <- substitute(x)
    env <- parent.frame()
    shiny::observe({
      tryCatch(
        eval(x, env),
        error = function(e) {
          showNotification(paste("Error: ", e$message), type = "error")
          # Can also do some logging here
        }
      )
    },
    ...
    )
  }
})
michaelhogersnplm commented 4 years ago

@wch thanks that is excellent, good idea to make it local to the app

D3SL commented 4 years ago

it seemed to me that you were saying that Shiny applications stop when they run into an error.

I was, but I did understand the difference between a silent error in an observer and a communicable error in a render function. That's why I linked the original thread on the RStudio site where it was brought up. But I understand where you were coming from now and I can appreciate what you meant.

You might already have done something like this, but one possible way of having something similar to a global switch is to define a version of observe that's local to the app.

That's a good idea that I might implement myself. But I still feel that Shiny has grown to a level where there needs to be some built in functionality to prioritize stability without requiring people hand-code their own stopgaps like this.

Having thought about it over the weekend I think the most workable solution is baking in an optional sub-argument like onerror() {error handling code}. If present and a non-communicable error occurs the shiny session follows the included code and continues, if not present then the session terminates as it does currently.

EG

observeEvent(input$add_row_button, {
    new_row<-tibble_row( 
    foo=input$foo
    bar=input$bar)
    bind_rows(reactivevals_container$current_reactive_data, new_row )

    onerror({
      sendSweetAlert(
      session = session,
      title = "Error...",
      text = #however you could grab and render error text in R
      type = "error"
      )
      lubridate::now() %>%
      paste0(.,"_data_dump.csv") %>%
      here::here() %>%
      write_csv(reactivevals_container$current_reactive_data, .)
      })

})

It avoids fragmentation via people altering Shiny namespace functions, it encourages thoughtful and defensive coding by requring people consider what errors may occur and how to handle them, and it preserves the default behavior so a broad userbase doesn't suddenly need to relearn basic habits. Plus the default behavior is a VERY immediately visible clue to new users that something went wrong in a way that is Very Bad(TM).

Also because all of this takes place inside an observer it even allows for robust logging and warnings. Someone's onerror() function could use shinywidgets to render a warning popup with an error code for example, or write a logfile and request the user save their work and safely terminate, or at the very minimum just dump all its data to disk and still shut down but be ready to pick back up without any loss of work.

I'd try to make the contrib myself but I'm nowhere near the level of skill to actually try to implement something like this.

D3SL commented 3 years ago

@wch If you can forgive the necrotag I've got a very belated followup, I'm not sure about github etiquette but it seemed posting here without reopening was better than making a new/duplicate issue. Before that however rereading this I didn't take enough care to convey passion for accessibility in a collegial tone and I apologize for that, nobody here deserved how unprofessional this sounded.

On the actual topic; Is it possible to use this same trick to define a error-logging versions of the observe and react functions that will follow your example by default but also optionally let someone define their own code to run for error = function(e){}?

Something like this:

catch_and_log_observer_errors <- TRUE

if (isTRUE(catch_and_log_observer_errors)) {
  observe <- function(observe_input,errormsg=showNotification(paste("Error: ", e$message), type = "error") ...) {
    observe_input <- substitute(observe_input)
    errormsg <- substitute(errormsg)
    env <- parent.frame()
    shiny::observe({
      tryCatch(
        eval(observe_input, env),
        error = function(e) {
          eval(errormsg, env)
          # Can also do some logging here
        }
      )
    },
    ...
    )
  }
})

which could be used like this:

observe({
  #code here
},
  errormsg = {
    #error handling code here
  }
)
mmuurr commented 3 years ago

Like others I've also thought about how to consistently handle conditions in observers where I didn't want application stoppage. In most cases to date, I've simply baked a tryCatch into the observer's expression, but this has the predominant drawback of leading to lots of code duplication in how signaled conditions are handled. The 'overridden' (or simply renamed) observer pattern discussed earlier in this thread solves a lot of that. ✅

But ... I've also considered a slightly more flexible approach, though haven't fully-committed to marching down the hairy NSE path of implementing just yet, and I figured I'd chime in to this thread to solicit thoughts.

Consider an augmented observe signature: try_observe(x, ..., handlers = list()), where ... are the existing observer args with defaults, and handlers is a list of named condition-handling functions, possibly with a finally expression. A tryCatch block then needs to be constructed programmatically and placed inside the underlying shiny::observe call, similar to the above. The very tricky NSE parts are making sure all expressions remain quoted when needed and are evaluated in the proper frames. For example, simply building a handlers list outside of the observer constructor call would require explicit quoting to prevent early evaluation of something like finally = { some_code_here() }. (The irony of preventing early-evaluation of finally is humorous enough to me to make this minimally a fun exercise :-)

In a default configuration with no handlers, try_observe simply behaves identically to observe: all conditions bubble up out of the observer's expression. But the advantage of such syntax could be concise specification of handlers for specific condition classes. Handling functions can be declared separately from the observers themselves:

handler.send_email_alert <- function(cond) { do_some_email(); magic_here(); }
handler.log_error <- function(cond) { logger::log_error(cond) }

Such handlers can even be created by closures (replicating something like factory methods) to build, e.g., loggers with specific destinations already pre-configured (I'm thinking of things like logging services, like Splunk, here).

Then, try_observe can be flexibly used in the application like so:

try_observe({
  something_ultra_important()
}, handlers = list(
  my.major.error.class = handler.send_email_alert,
  my.minor.error.class = handler.log_error,
  error = function() stop("unhandled error; big bada boom!")
)}

try_observe({
  something_less_important()
}, handlers = list(
  error = handler.log_error,
  finally = rlang::expr({
    not_really_sure_
    how_to_best_handle_
    quoting_expressions_
    quite_yet
  })
})

try_observe({
  without_any_handlers()  ## i.e. default handlers = list()
  this_behaves_identically()
  to_a_standard_observe()
})

In some light experimentation down this path, I've hit a few stumbling blocks in the careful dynamic assembly of the tryCatch expression itself, but I think this is more-or-less doable.

I'm curious to hear if anyone upstream in this discussion has thoughts on this approach, and whether or not something like this has been considered in the past.