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

(Refactoring) Simplify RMD child process disposal #773

Merged
merged 3 commits into from
Sep 6, 2021

Conversation

ElianHugh
Copy link
Collaborator

@ElianHugh ElianHugh commented Sep 6, 2021

Minor refactor to the R Markdown preview/knit processes, cleaning up the disposal of child processes.

Changes:

  • Contributes a helper function for creating disposables from objects
  • Simplifies disposal of child processes
  • Do not print lines to output when the process has already been terminated

Both knitting and previewing should function as before

@renkun-ken
Copy link
Member

I try the following Rmd

```{r}
x <- 1
Sys.sleep(100)
```

The kniting will take 100s to finish. When I click "Cancel", the progress window is closed but the R process is still running until it is finished. Shouldn't the process be killed once it is cancelled?

@ElianHugh
Copy link
Collaborator Author

ElianHugh commented Sep 6, 2021

Does the process not receive an exit signal in your output stream? This is what I see when I try it:

image

Edit: oh, I see what you mean, despite SIGKILL, the process output a file after 100 seconds. Bizarre, I'll try to see why

@renkun-ken
Copy link
Member

renkun-ken commented Sep 6, 2021

It does receive a SIGKILL signal


[VSC-R] terminating R process
[VSC-R] doc4.Rmd process exited from signal 'SIGKILL'

but I'm monitoring all R processes and the process is not actually terminated.

@ElianHugh
Copy link
Collaborator Author

I believe this is caused by the use of exec rather than spawn, as the shell appears to not close until completion

- Exec uses a shell which cannot be terminated on demand via SIGINT/SIGKILL
- terminating spawn appears to function appropriately
@ElianHugh
Copy link
Collaborator Author

Last push appears to have fixed the termination issue on my end

@renkun-ken
Copy link
Member

Thanks for the quick fix. It works on my end too.

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

- have to pass env and not quote rPath for windows spawn
@ElianHugh ElianHugh merged commit 1c643ad into REditorSupport:master Sep 6, 2021
@ElianHugh ElianHugh deleted the refactor-child-process branch September 6, 2021 09:52
ElianHugh added a commit to ElianHugh/vscode-R that referenced this pull request May 12, 2022
- Contributes a helper function for creating disposables from objects
- Simplifies disposal of child processes
- Do not print lines to output when the process has already been terminated
- use childProcess.spawn instead of childProcess.exec, to allow for faster process termination
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.

2 participants