Fix #2946: Ensure invalidation on freezeReactiveValue(input) freeze and new value

[jcheng5]

The goal of freezeReactiveValue is to protect reactives, observers, and (especially) outputs from seeing temporarily inconsistent states due to asynchronous operations that need to complete before the state can become consistent.

What is an inconsistent state?

Consider an app with two inputs: input$dataset and input$columns.

selectInput("dataset", "Choose a dataset", c("pressure", "cars"))
selectInput("column", "Choose column", character(0))

The columns selector should show the users the columns available from the input$dataset dataset. To maintain this relationship, we use an observer.

dataset <- reactive({
  switch(req(input$dataset),
    pressure = pressure,
    cars = cars,
    stop("Unexpected dataset value")
  )
})

observeEvent(input$dataset, {
  updateSelectInput(session, "column", choices = names(dataset()))
})

And here's an output that wants to use the dataset and column.

output$summary <- renderPrint({
  req(input$column)
  summary(dataset()[,input$column])
})

The problem occurs when input$dataset changes (also on first load, but let's focus on the change case), say from pressure to cars. The session updates input$dataset to cars, but the value of input$column is going to be a column from pressure still. So the renderPrint is going to try to access cars[,"temperature"] which will cause an error.

A moment later, the updateSelectInput takes effect in the client, causing input$column to be updated. renderPrint runs again, causing the error to disappear and a successful render to happen in its place.

The goal of freezeReactiveValue is to get rid of the flash.

observeEvent(input$dataset, {
  freezeReactiveValue(input, "column")
  updateSelectInput(session, "column", choices = names(dataset()))
})

The current implementation is supposed to work like this:

1. Updater calls freezeReactiveValue. This sets a frozen flag; while this flag is set, reads to the reactive value in question will throw the equivalent of req(FALSE).
2. Updater calls updateTextInput or renders a UI containing the same input or whatever.
3. When the current reactive flush cycle ends, all frozen values are automatically thawed.
4. Whatever operation the updater initiated finishes, and changes the reactive value, causing all consumers to invalidate.

However, there are several issues in the current (CRAN) implementation that make it subtly unreliable.

Bug 1: Ordering between updater and consumer matters

The first problem is that freezeReactiveValue doesn't ever trigger invalidation. If a consumer happens to run before an updater, then it can get into an error state, and freezing the reactive value afterwards won't notify the consumer. This is mostly a problem for outputs: it's not too bad for outputs to be in error states temporarily, but what's bad is for that error to make its way to the client.

1. input$dataset = "cars"
2. output$summary executes, resulting in an error
3. Updater executes, calling freezeReactiveValue()
4. (This is what's missing today:) output$summary re-executes, resulting in req(FALSE)

Ideally, updaters/outputs that call freezeReactiveValue would be set to a higher observer priority than any consumers. That would cause this not to be a problem at all, and it's also necessary to do this for non-output observers anyway (since an error can potentially be more serious in those cases).

Repro:

library(shiny)

ui <- fluidPage(
  selectInput("dataset", "Choose a dataset", c("pressure", "cars")),
  selectInput("column", "Choose column", character(0)),
  verbatimTextOutput("summary")
)

server <- function(input, output, session) {
  dataset <- reactive({
    switch(req(input$dataset),
      pressure = pressure,
      cars = cars,
      stop("Unexpected dataset value")
    )
  })
  
  observeEvent(input$dataset, {
    freezeReactiveValue(input, "column")
    updateSelectInput(session, "column", choices = names(dataset()))
  }, priority = -1)
  
  output$summary <- renderPrint({
    Sys.sleep(0.25)
    req(input$column)
    summary(dataset()[,input$column])
  })
}

shinyApp(ui, server)

Change the dataset and an error flashes by. I'm using priority = -1 to make the problem a little more persistent, but it can occur without it.

The fix to this problem is to invalidate upon freezeReactiveValue. Possible downsides are that existing apps could have consumers that either expensively or side-effect-ily execute after the invalidation occurs, i.e. they do slow or side-effecty stuff before attempting to read the frozen value.

With that fix, you still get an error in the R console but the user never sees the error in the browser.

Bug 2: Invalidation upon completion is unreliable due to dedupe

Similar scenario to last time, but we'll avoid Bug 1 and then go a little further.

1. input$dataset = "cars"
2. Updater executes, calling freezeReactiveValue() and updateSelectInput
3. output$summary executes, resulting in a req(FALSE)
4. Client sets numeric input
5. output$summary executes, resulting in success

The bug here is that step 5 can break down if updateSelectInput just so happens to set the input value to the same value it already has. In Shiny v1.5.0, that's a total no-op as far as the server is concerned.

Repro:

library(shiny)

ui <- fluidPage(
  selectInput("dataset", "Choose a dataset", c("pressure", "pressure1", "cars")),
  selectInput("column", "Choose column", character(0)),
  verbatimTextOutput("summary")
)

server <- function(input, output, session) {
  dataset <- reactive({
    switch(req(input$dataset),
      pressure = pressure,
      pressure1 = pressure,
      cars = cars,
      stop("Unexpected dataset value")
    )
  })
  
  observeEvent(input$dataset, {
    freezeReactiveValue(input, "column")
    updateSelectInput(session, "column", choices = names(dataset()))
  }, priority = 1)
  
  output$summary <- renderPrint({
    Sys.sleep(0.25)
    req(input$column)
    summary(dataset()[,input$column])
  })
}

shinyApp(ui, server)

Change the dataset from "pressure" to "pressure1" and you'll see the summary go blank.

The fix for this is that calling freezeReactiveValue should disable "ignore duplicate values" for the very next set. This needs to be implemented one way for input, another way for reactiveValues, and a third way for reactiveVal.

Bug 3: #2946 Downstream reactive expressions don't react to their dependencies being frozen

The issue discusses it pretty well. Basically, invalidation on freeze is needed--same fix as for bug 1.

[jcheng5]

OK. This PR now represents sort of a compromise, in an effort to fix the problems we know are actually happening to people, without attempting to fix problems that are theoretical and may not affect anyone (because attempting to fix the latter could conceivably break compatibility if people have figured out clever ways to make use of freeze/thaw).

1. freezeReactiveValue(input) now invalidates on freeze. (freezeReactiveValue(rv) and freezeReactiveVal() are unchanged.)
2. freezeReactiveValue(input) now causes the client to NOT dedupe the very next setting of the input. This ensures that reactive dependencies that read the frozen value, are guaranteed to get invalidated when it's safe to un-freeze.
3. freezeReactiveValue(rv) and freezeReactiveVal() emit deprecation messages, directing people to Deprecate freezeReactiveVal, and freezeReactiveValue for non-inputs #3063. The goal is to either get enough feedback to let us confidently apply the same fix to those scenarios, or, if nobody cares then we just drop that functionality.

Future work:

1. I haven't thought about observeEvent(input$not_frozen, { input$frozen }), hmmm, that sucks.
2. I would love not to thaw automatically at the end of the the flush cycle, it feels so arbitrary.