-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
protect R objects cached for data viewer #14657
Conversation
6e7551a
to
1314ef3
Compare
client <- remote$client | ||
|
||
# https://github.com/rstudio/rstudio/pull/14657 | ||
test_that("we can use the data viewer with temporary R expressions", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's a more appropriate test, but at least this exercises the underlying code.
@@ -448,9 +481,6 @@ | |||
.rs.setVar("automation.client", client) | |||
.rs.setVar("automation.sessionId", sessionId) | |||
|
|||
# Navigate the page to RStudio Server. | |||
client$Page.navigate("http://localhost:8787") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer necessary as we now point the browser directly at the running RStudio Server instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Intent
Addresses #14643.
Approach
Now that we try to introspect the contents of the observed R object shown in the data viewer, we need to protect that object from the GC.
Automated Tests
A basic automation test is included, but unfortunately is not sufficient to capture the underlying crash due to the stochastic nature of the problem.
QA Notes
N/A
Documentation
N/A
Checklist
NEWS.md