builtin_handler_shiny results in Error: attempt to apply non-function

[dipterix]

Dear cli maintainer, I recently encountered an issue ropensci/targets#1478 (please check this issue for more detailed examples)

The targets package probably tried using cli progress bar before initialized ("add") or after completion, resulting in an error. The related code is here

https://github.com./r-lib/cli/blob/main/R/progress-server.R#L356C1-L365C5

We see that shiny progress is set to NULL, and then trying to access functions such as output or set will raise errors.

bar$shiny_progress <- NULL

Basically if shiny ends the progress bar builtin_handler_shiny$complete() and the program still tries to call builtin_handler_shiny $output(), then the code will error out in shiny app. However, the code works fine outside of shiny, which makes the debugging process extremely hard.

I understand that if the package is used super carefully, then this issue will not occur. However, it is often the case that the package does not mandate what people do, especially when the code just works for most of people. Considering that other handlers do not raise errors on "use-after-free", and condition that triggers this error, this inconsistency is super hard to debug/find.

However, could we add checks just to make sure the functions are not that stringent? Alternatively, if you would like to ensure consistent behavior, we can force set and output to result in errors after completion for other functions.

builtin_handler_shiny <- list(
  able = function(bar, .envir) {
    "shiny" %in% loadedNamespaces() && asNamespace("shiny")$isRunning()
  },

  add = function(bar, .envir) {
    bar$shiny_progress <- asNamespace("shiny")$Progress$new(
      asNamespace("shiny")$getDefaultReactiveDomain(),
      min = 0,
      max = bar$total
    )
    bar$shiny_progress$set(
      message = bar$name %||% "",
      detail = shiny_detail(bar, .envir)
    )
  },

  set = function(bar, .envir) {
    if(!is.null(bar$shiny_progress)) {
      bar$shiny_progress$set(
        value = bar$current,
        detail = shiny_detail(bar, .envir)
      )
    }
  },

  complete = function(bar, .envir, results) {
    if (!is.null(bar$shiny_progress)) {
      bar$shiny_progress$set(
        value = bar$current,
        detail = shiny_detail(bar, .envir)
      )
      bar$shiny_progress$close()
    }
    bar$shiny_progress <- NULL
  },

  output = function(bar, .envir, text) {
    bar$shiny_output <-
      last_lines(paste0(bar$shiny_output, " \u2022 ", text))
    if(!is.null(bar$shiny_progress)) {
      bar$shiny_progress$set(
        value = bar$current,
        detail = shiny_detail(bar, .envir)
      )
    }
  }
)

[gaborcsardi]

Can you try to create an example to show this?

[dipterix]

I will use the example from the issue ropensci/targets#1478. Notice this might not be the "supposed" way of using cli, but it's quite hard to find this bug as it only occurs in Shiny

shiny::shinyApp(
  ui = shiny::actionButton("run", "run"),
  server = function(input, output, session) {
    shiny::bindEvent(
      shiny::observe({
        envir <- new.env(parent = globalenv())
        id <- cli::cli_progress_bar()
        cli::cli_progress_output("custom message", .envir = envir, id = id)
      }),
      input$run,
      ignoreInit = TRUE
    )
  }
)