Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack trace domain explosion #4155

Merged
merged 5 commits into from
Dec 3, 2024
Merged

Stack trace domain explosion #4155

merged 5 commits into from
Dec 3, 2024

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Nov 27, 2024

This is part of the fix for https://github.com/jcheng5/shinychat/issues/16

Simple repro (watch the dots get slower and slower):

library(shiny)
library(promises)
library(coro)

ui <- fluidPage(
  
)

server <- function(input, output, session) {
  observe({
    coro::async(function () {
      for (i in 1:1e3) {
        await(coro::async_sleep(0))
        cat(".")
        if (i %% 50 == 0) cat("\n")
      }
    })()
  })
}

shinyApp(ui, server)

Using `captureStackTraces` in wrapForContext is a bad idea, it
piles on a new domain every time a handler is bound.
@jcheng5 jcheng5 added this to the Next Release milestone Dec 2, 2024
The `category` column isn't a good candidate for snapshot
testing, as its contents vary depending on how the package
was loaded/installed. During devtools::test() or similar,
shiny package code shows up as 'user'. But during CI, it
doesn't show up as anything.
@jcheng5 jcheng5 marked this pull request as ready for review December 3, 2024 01:50
@jcheng5 jcheng5 requested a review from cpsievert December 3, 2024 01:50
@jcheng5
Copy link
Member Author

jcheng5 commented Dec 3, 2024

@cpsievert will review post-merge

@jcheng5 jcheng5 merged commit 4589245 into main Dec 3, 2024
12 checks passed
@jcheng5 jcheng5 deleted the stack-trace-domain-explosion branch December 3, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant