add support for global session ended callbacks

[kevinushey]

The main intent here is to allow RStudio (or, other front-ends) to add their own global "on connection closed" callback hooks, which could be used to allow the front-end to respond if the client connection is closed.

More concretely, in RStudio, this allows us to present a Shiny application to the user in an RStudio window, and stop the application when they close that window.

I didn't add anything to NEWS as this is sort of inside baseball, but let me know if this is worth documenting as a public interface.

(CPS: note this is in support of https://github.com/rstudio/rstudio/issues/13394)

[CLAassistant]

All committers have signed the CLA.

[cpsievert]

Thanks @kevinushey. Would it make sense for onSessionEnded() to take inspiration from onStop() in the sense that, if it gets used outside of a session context, it registers a global callback?

[kevinushey]

What would the behavior be if onSessionEnded is called multiple times? Do we allow for a stack of callbacks, or does only the most-recently registered one apply?

[cpsievert]

Looks like onStop() supports multiple callbacks, so it probably makes sense to be consistent with that?

onStop(\() print("one"))
onStop(\() print("two"))
shinyApp(basicPage(), \(...) {})
#> [1] "one"
#> [1] "two"

[kevinushey]

@cpsievert would you be able to help me get this over the finish line?

[cpsievert]

@kevinushey I'd be happy to help. Are you able to share the callback you had in mind for addressing https://github.com/rstudio/rstudio/issues/13394? Just hoping to gain a better understanding of the situation...

Also, I took a quick stab at allowing onSessionEnded() to be called outside of a session in 0cca7a6. We don't have to go in this direction, but it does seem like what you're wanting here could be supported in this more public-facing way.

Note also that, the only difference between this approach to onSessionEnded() and onStop() is the point at when they execute (the former happens when the websocket closes and the later when the runApp() function exits). Before we consider adding this feature, I'd just want to confirm that the timing of onStop() doesn't already work for your use case.

[kevinushey]

@kevinushey I'd be happy to help. Are you able to share the callback you had in mind for addressing https://github.com/rstudio/rstudio/issues/13394? Just hoping to gain a better understanding of the situation...

Sure -- just to give the full context, when someone clicks Run App for a Shiny application in RStudio, we try to open the running application in a new RStudio window (if configured to do so). When that window is closed, we want to stop the running application. Currently, this is accomplished by sending an interrupt to the R console, but this has the unfortunate side effect of also interrupting the execution of any onSessionEnded() callbacks, which some applications might rely on.

With this PR, I think we could avoid sending an interrupt, and instead run these applications with an onSessionEnded() callback that would call shiny::stopApp() to stop the application after the window is closed.

Orchestrating this through a global onSessionEnded() callback seemed like the most straightforward approach, but I'm open to alternatives if you think there might be a better way forward.

[wch]

I think it would be good if we could get rid of the interrupt. However, there may be some problems with doing it via an onSessionEnded callback.

We don't want it to call onStop() when just any session closes. Currently, in RStudio, if you close the Shiny App Viewer or Shiny App Window, it sends the interrupt to R, and the app will stop. But if you close an external browser window to the app, it will not send an interrupt to R, and the app will keep running. I don't think we want to change this behavior, although I could be wrong. (For development, I find it useful to be able to open and close multiple windows that all use a the same running app.)

So if we had a callback that ran when any session ended, we would want that callback to be able to detect if the session was ended from closing the App Viewer or App Window, and not from closing an external browser window. @kevinushey do you know of a way that information could be conveyed?

[kevinushey]

On the RStudio side, this is mainly to support users who run their Shiny application by clicking the "Run App" button. My thought is we'd do something like this -- when the user clicks Run App,

1. Check if the user has requested the application be shown in an RStudio window,
2. If so, set the global "quit-on-session-exit" callback,
3. Run the application via shiny::runApp() (or the equivalent call),
4. When the window is closed, the "quit-on-exit-callback" is invoked; that callback also takes care of removing itself on exit.

Another alternative that might be worth considering: could Shiny register a Javascript message handler, and listen for a "quit" message from RStudio (that would be fired when the window is closed)? I'm not sure if there's a way to do that safely / prevent malicious outside use, though.

[wch]

After a brief discussion with @kevinushey it looks like RStudio might be able to call stopApp() when the window is closed, instead of sending an interrupt to the R process. If that's the case, then Shiny might not need any changes.

If that doesn't work out, then there is another way that we could change RStudio and Shiny to make this work, which I can explain if needed.

[kevinushey]

Just confirming that this indeed does seem to work. There is still a small downside -- users can still manually interrupt R themselves while session callbacks are running, which will then lead to this error being emitted:

Error: invoke_wrapped: throwing std::runtime_error

So, an alternate change might still be worth considering -- should Shiny suspend interrupts while running session callbacks? I could see arguments in both directions; e.g. maybe the onus is on the user to suspend interrupts themselves if running those session callbacks is very important...

[wch]

I think it would actually be more consistent with the rest of Shiny to not suspend interrupts when executing the session-end callbacks. All other app code in a Shiny app can be interrupted, and this can be useful -- if they're doing something wrong and hanging, it can be useful to interrupt it.

If there are parts app that really shouldn't be interrupted (whether in a session-end callback or not) then the app author can wrap the critical sections in suspendInterrupts({ ... }).

[kevinushey]

I think you're right. It's probably worth documenting somewhere that Shiny application authors might want to use suspendInterrupts() for some set of "essential" computations but I don't know where that would live.

[kevinushey]

All of that said... are global session callbacks something that might still be useful more generally? Or should we just close this PR?

[wch]

I think it would be best to just close this PR -- if another use case comes up, then we can come back to it.