rstudio / shiny

Easy interactive web applications with R
https://shiny.posit.co/
Other
5.35k stars 1.87k forks source link

Infinite loop with `conditionalPanel` #4126

Open kamilzyla opened 1 week ago

kamilzyla commented 1 week ago

Example app

This app prints dots indefinitely:

shinyApp(
  ui = div(
    id = "box",
    conditionalPanel(condition = "undefined"),
    tags$script(HTML("
      $('#box').on('hide', () => {
        Shiny.setInputValue('dot', Math.random());
      });
    "))
  ),
  server = function(input, output) {
    observeEvent(input$dot, cat("."))
  }
)

Details

The falsy condition in conditionalPanel causes it to trigger a hide event, upon which we update the dot input. This causes the condition to be evaluated again and another event to be fired, resulting in an endless loop.

It works because this check ("Is the desired state different from the current state?") always evaluates to true if the condition passed to conditionalPanel is not a boolean. Replacing undefined with 42 and hide with show also results in an endless loop.

Fix

My suggestion is to fix the problem by coercing the condition to a bool in line 617, like so:

const show = !!condFunc(nsScope);

A workaround for user code is to ensure that condition evaluates to a boolean (e.g. with !!).

Notes

Checked with Shiny 1.9.1, but looking at git history the problem has been around for a while.

Perhaps this is not something many people will encounter, but in our case this bug was causing performance issues and it was very hard to find it (the conditionalPanel() and the event listener were pieces of two unrelated functionalities in a complex app).

gadenbuie commented 1 week ago

Thanks @kamilzyla, I can reproduce the issue even on shinylive.io.

We definitely want to avoid the constant stream of events and I think you're right in your fix. Note that in theory we've typed scopeExprToFunc() (which creates the condFunc()) as returning boolean, so in theory the fix could be applied in there. That said, I think scopeExprToFunc() would be better typed as unknown because of its general application; alternatively we could force it to return a Boolean value and rename it scopeExprToPredicateFunc(). I'm going to PR the type change to unknown.

Thanks again for finding this issue and reporting it with a clear reprex and suggestion!