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

Ensure workspace is cleared #1318

Merged
merged 6 commits into from
Mar 5, 2023

Conversation

ElianHugh
Copy link
Collaborator

@ElianHugh ElianHugh commented Mar 2, 2023

What problem did you solve?

  • Workspace viewer will no longer show "ghost" data from previous sessions. Global env, search, and loaded namespaces will all be cleared when an attached term is closed.
  • Status bar item will be reset to 'R: (not attached)'

Workspaces will be cleared if:

  • R is quit safely
  • An attached integrated terminal is closed

(If you have)Screenshot

1

After closing the terminal:

2

Workspace viewer will no longer show "ghost" data from previous sessions
@renkun-ken
Copy link
Member

renkun-ken commented Mar 2, 2023

Thanks for working on this!

Ideally, if the R session exits, the workspace viewer should go empty, and the status bar item should be reset to "R: (not attached)".

In my case, I mostly use self-managed R terminals (e.g. starting radian in tmux windows manually). Not sure if there is a way to handle this so that whenever the attached R session exits, the workspace viewer and status bar item will be reset.

@ElianHugh
Copy link
Collaborator Author

ElianHugh commented Mar 2, 2023

Hmm, that's a good point, thank you. I wonder if using a finalizer on the R side of things could be viable... will have a play around with it

@renkun-ken
Copy link
Member

I tried reg.finalizer to the global environment before, it seems to only work if the R process is exited by q() but not killed externally or crashed.

A simple way is to add the following in vsc.R:

reg.finalizer(globalenv(), function(e) .vsc$request("detach"), onexit = TRUE)

and handle detach request on extension side by resetting workspace viewer and status bar item.

@ElianHugh
Copy link
Collaborator Author

I'm not sure there's a way to handle unsafe exits -- couldn't find any way to handle them. Even temp folders don't seem to be cleared when R crashes or is killed

I think having a finalizer would still be worth it in the long run? I'd like a solution for unsafe exits though

@renkun-ken
Copy link
Member

Not sure if there is a way to get notified by the exit signal of an external process in nodejs since we know the pid of the attached R session.

@ElianHugh
Copy link
Collaborator Author

Good idea! Looks like it is possible to check if an arbitrary process is alive via process.kill(pid, 0). See: https://stackoverflow.com/questions/14390930/how-to-check-if-an-arbitrary-pid-is-running-using-node-js

Not exactly a watcher though

- safe exit of R session causes a detach
- detach clears workspace + status item
@renkun-ken
Copy link
Member

The process.kill(pid, 0) approach seems good enough for session watcher to check every 1 or more seconds once attached so that we don't have to rely on the R session to report its exiting?

@ElianHugh
Copy link
Collaborator Author

Hi @renkun-ken, would you like to try the current implementation to see if it works with your tmux setup?

@renkun-ken
Copy link
Member

renkun-ken commented Mar 5, 2023

I tried with the following use cases and all work properly!

  • "Create R terminal" and remove terminal
  • Manually start an R session, attach, and q().
  • Manually start an R session, Sys.getpid(), and kill <pid> externally.

Just push a minor fix to address the linter.

Copy link
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@renkun-ken renkun-ken merged commit 3a0784f into REditorSupport:master Mar 5, 2023
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