Skip to content

Use spawn#985

Merged
renkun-ken merged 14 commits intoREditorSupport:masterfrom
renkun-ken:use-spawn
Feb 11, 2022
Merged

Use spawn#985
renkun-ken merged 14 commits intoREditorSupport:masterfrom
renkun-ken:use-spawn

Conversation

@renkun-ken
Copy link
Copy Markdown
Member

What problem did you solve?

This PR makes use of spawn and spawnSync across the entire project. In particular, getAliases is updated to use spawnSync to be consistent with others.

@ManuelHentschel
Copy link
Copy Markdown
Member

Thanks for the work. If I remember correctly, we only use a file to receive the results of getAliases.R, since there were some buffer overflow issues with exec. These should not be an issue with spawn according to e.g. https://stackoverflow.com/a/56688608, so I it might make sense to use stdout again. If you agree, I could implement that later today.

@renkun-ken
Copy link
Copy Markdown
Member Author

renkun-ken commented Feb 10, 2022

Personally, I prefer receiving small data (a file path, a small chunk of text, etc.) directly from stdout whereas large data (all aliases, all rmd template info, etc.) from a temp file. Here we use spawn and spawnSync for consistency across the project in terms of command argument quoting, shell wrapping, etc.

Does that make sense?

@ManuelHentschel
Copy link
Copy Markdown
Member

If there are no issues with buffer overflows, are there advantages to using a file over stdout? We load the entire file into memory at some point, anyways, and using temp files always has some potential to cause problems with cleanup, access restrictions, or even performance (on very limited systems).

@renkun-ken
Copy link
Copy Markdown
Member Author

It looks like spawnSync also has a buffer size limit yet it could be adjusted. From spawnSync documentation, the buffer size could be specified in options:

maxBuffer: Largest amount of data in bytes allowed on stdout or stderr. If exceeded, the child process is terminated and any output is truncated. See caveat at maxBuffer and Unicode. Default: 1024 * 1024.

Do you mean we better use spawn and collect the stdout until the process exits?

@ManuelHentschel
Copy link
Copy Markdown
Member

It looks like spawnSync also has a buffer size limit yet it could be adjusted. From spawnSync documentation, the buffer size could be specified in options:

maxBuffer: Largest amount of data in bytes allowed on stdout or stderr. If exceeded, the child process is terminated and any output is truncated. See caveat at maxBuffer and Unicode. Default: 1024 * 1024.

Do you mean we better use spawn and collect the stdout until the process exits?

Yes, that's what I had in mind (assuming there is no buffer with the async version)

@renkun-ken
Copy link
Copy Markdown
Member Author

Now I switch to using spawn.

Comment thread src/helpViewer/helpProvider.ts Outdated
this.rScriptFile
];

return new Promise((resolve) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason that we show some errors to the user and log the errors in the childprocess/non-matching regex only to the console?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now an error message is shown to user.

I'm wondering if it makes sense to use a global output channel to show details of such information? But we already have multiple output channels: R Language Server, R Markdown, and VSCode-R-Debugger also has R Debugger.

Comment thread src/helpViewer/helpProvider.ts Outdated
Comment thread src/helpViewer/helpProvider.ts Outdated
Comment thread src/helpViewer/helpProvider.ts Outdated
Comment thread src/helpViewer/helpProvider.ts Outdated
@renkun-ken renkun-ken merged commit aaf809d into REditorSupport:master Feb 11, 2022
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