[RDY] Refactor os_call_shell to use os_system #1365

tarruda · 2014-10-31T10:17:56Z

Should close #473

aktau · 2014-10-31T11:53:47Z

Would it be feasible to do a check at nvim exit time to see if all handles have been properly cleaned up (in debug mode at least)?

tarruda · 2014-10-31T12:20:00Z

Would it be feasible to do a check at nvim exit time to see if all handles have been properly cleaned up (in debug mode at least)?

I think the proper way to do this is to call uv_run(uv_default_loo(), UV_RUN_DEFAULT) at exit, it will return immediately if no watchers are set.

But if you are talking about the first commit in this PR, the problem is much bigger: Due to the way we interact with libuv, leaving unclosed, stack-allocated handles can have really bad effects, such as a smashed stack. I detected the error caused by those unclosed handles whend debugging #1328

splinterofchaos · 2014-10-31T16:32:38Z

Does this close #473 or do you still want it to allow custom callback? I ask because I've found nice performance increases by doing so (#1263/#1241).

tarruda · 2014-10-31T16:36:56Z

Does this close #473 or do you still want it to allow custom callback? I ask because I've found nice performance increases by doing so (#1263/#1241).

I had forgot about those details! Do you mind rebasing your work on top of this PR?

tarruda · 2014-10-31T16:51:07Z

Does this close #473 or do you still want it to allow custom callback? I ask because I've found nice performance increases by doing so (#1263/#1241).

os_call_shell is now pretty close to os_system, the biggest difference is the logic to read/write from/to the current buffer. Could the performance improvement you noticed be the consequence of avoiding temporary file redirection when constructing the shell command?

splinterofchaos · 2014-10-31T16:55:09Z

I'd be happy to .:)

A little off topic, but #1241 is blocked on not being able to dup stderr and stdout to a single stream. Do you happen to know a way? (Not that I didn't already ask on the mailing list: https://groups.google.com/forum/#!topic/libuv/Q3espD5WWw8)

tarruda · 2014-10-31T17:04:08Z

A little off topic, but #1241 is blocked on not being able to dup stderr and stdout to a single stream

Wouldn't that be the same as reading from both streams and appending to a single buffer? That is what os_system does. Maybe you can get a similar performance in #1241 by using os_system to implement make, grep, etc(instead of patching os_call_shell with callbacks). If required, feel free to change os_system to accept extra shell arg, etc

splinterofchaos · 2014-10-31T17:07:13Z

Could the performance improvement you noticed be the consequence of avoiding temporary file redirection when constructing the shell command?

I tested :grep and :make with a couple different strategies and here's a list from slowest to fastest (unfortunately, I don't have the raw data anymore):

The current method.
os_system()
job_start()
The current method, but with mkfifo().

Since (4) isn't portable, I figured (3) would be best, but os_system() is indeed an improvement. The advantage to (3) and (4) is that programs like make and ag often have noticeable pauses in their output, so by the time they finish, we can have half the data already processed.

Wouldn't that be the same as reading from both streams and appending to a single buffer?

When I tested :grep with a job, I found the errors ("* is a dir") all bunched together even though stdout and err ran through the same function. Though, I just tested :call system('make') and it looks alright. I might want to try again.

tarruda · 2014-10-31T17:32:15Z

I tested :grep and :make with a couple different strategies and here's a list from slowest to fastest (unfortunately, I don't have the raw data anymore):

The current method.

os_system()

job_start()

The current method, but with mkfifo().

Since (4) isn't portable, I figured (3) would be best, but os_system() is indeed an improvement. The advantage to (3) and (4) is that programs like make and ag often have noticeable pauses in their output, so by the time they finish, we can have half the data already processed.

If that's the case, maybe we should simply leave os_call_shell as it is and use job_start directly? It has the advantage of skipping an additional process spawn(the shell)

splinterofchaos · 2014-10-31T19:50:02Z

maybe we should simply leave os_call_shell as it is and use job_start directly?

Things like :grep dir/* won't work. Though, as an optimization, we could check mch_has_wildcard() first.

splinterofchaos · 2014-11-01T01:42:35Z

On second thought, it would be pretty trivial to just call job_start() with sh -c "grep ...".

Commit @709685b4612f4 removed the close_job_* calls when uv_spawn fails because of memory errors when trying to cleanup unitialized {R,W}Stream instances, but the uv_pipe_t instances must be closed because they are added to the event loop queue by previous `uv_pipe_init()` calls

Passing NULL as the callback for stdout/stderr will result in job_start ignoring stdout/stderr, respectively. A 'writable' boolean argument was also added, and when false `job_start` will ignore stdin. Also, refactor os_system to allow passing NULL as the `output` argument.

To follow our coding conventions

Use a timer to periodically compare the current HR time against the HR time of when `job_stop` was called. After 1 second, send SIGTERM, after 2 seconds, send SIGKILL. The timer is only active when there's at least one `job_stop` call pending.

aktau · 2014-11-03T17:02:09Z

But of a late review, but apart from my one minor niggle noted in-place, this seems legit (and a good simplification).

oni-link · 2015-01-06T22:55:33Z

src/nvim/os/shell.c

-  // close the input stream, let the process know that no more input is coming
-  job_close_in(job);
-  int status = job_wait(job, -1);
+  status = job_wait(job, -1);


We can return early from job_wait() (hitting CTRL-C) and there can still be waiting callbacks to system_data_cb().
If shell() returns, the stack allocated buffer buf becomes invalid, but the job callbacks can still try to use buf.

jszakmeister added the in progress label Oct 31, 2014

tarruda mentioned this pull request Oct 31, 2014

jobs: wstream segfault and SIGPIPE #1363

Closed

tarruda force-pushed the shell-job-refactor branch 3 times, most recently from 02e0370 to ac599de Compare October 31, 2014 10:52

tarruda force-pushed the shell-job-refactor branch from ac599de to 02c5d8b Compare October 31, 2014 12:14

tarruda force-pushed the shell-job-refactor branch 7 times, most recently from 63efb1c to 4274467 Compare October 31, 2014 16:26

tarruda force-pushed the shell-job-refactor branch from 4274467 to efb8e7c Compare October 31, 2014 17:40

tarruda changed the title ~~[WIP] Refactor os_call_shell to use os_system~~ [RDY] Refactor os_call_shell to use os_system Oct 31, 2014

tarruda added 4 commits October 31, 2014 22:52

shell: Rename dyn_buffer_t to DynamicBuffer

6801536

To follow our coding conventions

rstream: Add rstream_buffer method to get a reference to RBuffer

88c4747

tarruda added 3 commits October 31, 2014 22:52

job/shell: Refactor os_call_shell/os_system to share code

01761cd

job: Fix job_wait to properly cleanup the job when it exits.

d878569

tarruda force-pushed the shell-job-refactor branch from efb8e7c to d878569 Compare November 1, 2014 01:52

tarruda merged commit d878569 into neovim:master Nov 1, 2014

tarruda added a commit that referenced this pull request Nov 1, 2014

Merge PR #1365 'Refactor os_call_shell to use os_system'

591391b

tarruda mentioned this pull request Nov 3, 2014

do_filter() behaves differently under nvim than vim (less doesn't work) #1386

Closed

oni-link reviewed Jan 6, 2015
View reviewed changes

This was referenced Jan 8, 2015

^C no longer interrupts long running shell commands. #1234

Closed

[RFC] job.c: Prevent early return from job_wait(). #1798

Merged

tarruda mentioned this pull request Feb 28, 2015

[RDY] Builtin terminal emulation #2076

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[RDY] Refactor os_call_shell to use os_system #1365

[RDY] Refactor os_call_shell to use os_system #1365

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

aktau commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

splinterofchaos commented Nov 1, 2014

Uh oh!

aktau commented Nov 3, 2014

Uh oh!

oni-link Jan 6, 2015

Uh oh!

Uh oh!

Uh oh!

[RDY] Refactor os_call_shell to use os_system #1365

[RDY] Refactor os_call_shell to use os_system #1365

Uh oh!

Conversation

tarruda commented Oct 31, 2014

Uh oh!

aktau commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

tarruda commented Oct 31, 2014

Uh oh!

splinterofchaos commented Oct 31, 2014

Uh oh!

splinterofchaos commented Nov 1, 2014

Uh oh!

aktau commented Nov 3, 2014

Uh oh!

oni-link Jan 6, 2015

Choose a reason for hiding this comment

Uh oh!

Uh oh!