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

protect R objects cached for data viewer #14657

Merged
merged 5 commits into from
May 15, 2024

Conversation

kevinushey
Copy link
Contributor

@kevinushey kevinushey commented May 6, 2024

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

  • If this PR adds a new feature, or fixes a bug in a previously released version, it includes an entry in NEWS.md
  • If this PR adds or changes UI, the updated UI meets accessibility standards
  • A reviewer is assigned to this PR (if unsure who to assign, check Area Owners list)
  • This PR passes all local unit tests

@kevinushey kevinushey force-pushed the bugfix/data-viewer-observed-sexp branch from 6e7551a to 1314ef3 Compare May 14, 2024 20:53
@kevinushey kevinushey requested a review from gtritchie May 14, 2024 21:45
@kevinushey kevinushey marked this pull request as ready for review May 14, 2024 21:45
@kevinushey kevinushey changed the title [WIP] protect R objects cached for data viewer protect R objects cached for data viewer May 14, 2024
client <- remote$client

# https://github.com/rstudio/rstudio/pull/14657
test_that("we can use the data viewer with temporary R expressions", {
Copy link
Contributor Author

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")
Copy link
Contributor Author

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.

Copy link
Member

@gtritchie gtritchie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@gtritchie gtritchie merged commit 7f2fc2f into main May 15, 2024
2 checks passed
@gtritchie gtritchie deleted the bugfix/data-viewer-observed-sexp branch May 15, 2024 15:12
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.

None yet

2 participants