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

[RFC] Reimplement job control patch with libuv #475

Merged
merged 1 commit into from Apr 7, 2014

Conversation

Projects
None yet
5 participants
@tarruda
Member

tarruda commented Apr 5, 2014

This is an improved version of the job control patch I sent to vim mailing list a few months ago, and it will be the base for the new plugin architecture. The job module implemented here will be reused for spawning plugins.

The basic difference between plugins and plain jobs started with jobstart is that instead of invoking an auto commands passing the raw data to the vimscript programmer, plugins will have access to Neovim msgpack API directly

This is still a WIP so I'm still going to fix documentation, formatting, etc(this was basically a copy and paste from the patch)

@schmee

This comment has been minimized.

Show comment
Hide comment
@schmee

schmee Apr 5, 2014

Contributor

Great to see the new plugin architecture taking form! 🎉

Contributor

schmee commented Apr 5, 2014

Great to see the new plugin architecture taking form! 🎉

Show outdated Hide outdated src/os/job.c
@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 6, 2014

Member

Couldn't wait, so I just tested it. It almost works :)

In case it's of any value, I get Abort trap: 6 by the following steps:

Create a simple bash script:

$ echo "sleep 3 && exit 42" > test.sh && chmod u+x test.sh

Do the following in nvim:

:au JobActivity * echom string(v:job_data)
:call jobstart('foo', '/Users/justin/test.sh') 
Member

justinmk commented Apr 6, 2014

Couldn't wait, so I just tested it. It almost works :)

In case it's of any value, I get Abort trap: 6 by the following steps:

Create a simple bash script:

$ echo "sleep 3 && exit 42" > test.sh && chmod u+x test.sh

Do the following in nvim:

:au JobActivity * echom string(v:job_data)
:call jobstart('foo', '/Users/justin/test.sh') 
@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 6, 2014

Member

@justinmk this last commit should have fixed it. Here's something else you can use to play with it:

let g:nc = jobstart('netcat', 'nc', ['-l', '1234'])
au JobActivity netcat call append(line('$'), v:job_data[1])

Open another terminal and type the following:

nc localhost 1234

and start entering lines. Also calling jobwrite(g:nc, str) should write str to the terminal running the netcat client.

Note that I'm still not handling the redraw issue correctly, for now it's simply calling shell_resized() which is sub-optimal.(Still need to figure out the right functions to call for redrawing only the invalidated columns)

Member

tarruda commented Apr 6, 2014

@justinmk this last commit should have fixed it. Here's something else you can use to play with it:

let g:nc = jobstart('netcat', 'nc', ['-l', '1234'])
au JobActivity netcat call append(line('$'), v:job_data[1])

Open another terminal and type the following:

nc localhost 1234

and start entering lines. Also calling jobwrite(g:nc, str) should write str to the terminal running the netcat client.

Note that I'm still not handling the redraw issue correctly, for now it's simply calling shell_resized() which is sub-optimal.(Still need to figure out the right functions to call for redrawing only the invalidated columns)

Show outdated Hide outdated src/os/job.h
@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 6, 2014

Member

I'm curious, is there some "rule" against usage of goto statements? Many say it's always a bad practice, but personally I think they are a simple way to perform cleanup in a function(Currently using it in f_job_start. One can also extract cleanup code into a separate function like I have done here but I think it's a bit of overkill. Here is an interesting article about the subject.

I think this is something we might want to establish in our code style, as I said I'm ok with gotos for cleanup code.

Member

tarruda commented Apr 6, 2014

I'm curious, is there some "rule" against usage of goto statements? Many say it's always a bad practice, but personally I think they are a simple way to perform cleanup in a function(Currently using it in f_job_start. One can also extract cleanup code into a separate function like I have done here but I think it's a bit of overkill. Here is an interesting article about the subject.

I think this is something we might want to establish in our code style, as I said I'm ok with gotos for cleanup code.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 6, 2014

Member

@tarruda The dogmatism surrounding anti-goto sentiment is cargo-cult adherence to Dijktra's famous "goto considered harmful" essay. It's still the right tool for C occasionally.

The essay was originally written in the context of trying to convince assembly programmers of the 1970s that function dispatch was not going to kill performance.

One can also extract cleanup code into a separate function like I have done here but I think it's a bit of overkill

Exactly. In languages with private functions (and no forward declaration requirement) and RAII or garbage collection, goto is a code smell. But C doesn't have those things, so AFAIK goto is acceptable in some limited cases.

Member

justinmk commented Apr 6, 2014

@tarruda The dogmatism surrounding anti-goto sentiment is cargo-cult adherence to Dijktra's famous "goto considered harmful" essay. It's still the right tool for C occasionally.

The essay was originally written in the context of trying to convince assembly programmers of the 1970s that function dispatch was not going to kill performance.

One can also extract cleanup code into a separate function like I have done here but I think it's a bit of overkill

Exactly. In languages with private functions (and no forward declaration requirement) and RAII or garbage collection, goto is a code smell. But C doesn't have those things, so AFAIK goto is acceptable in some limited cases.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 7, 2014

Member

@justinmk thanks for the link, interesting read :)

Member

tarruda commented Apr 7, 2014

@justinmk thanks for the link, interesting read :)

@tarruda tarruda changed the title from [WIP] Reimplement job control patch with libuv to [RFC] Reimplement job control patch with libuv Apr 7, 2014

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 7, 2014

Member

I'm not satisfied with having to call shell_resized after handling each job event. The problem seems to be that the whole redrawing logic is scattered across edit.c/normal.c(not sure though, I have tried playing with the functions in screen.c but couldn't find anything obvious) and is executed whenever keys are pressed. The shell_resized call can be removed by using the 'events as keys' hack I proposed in #395, this will make vim redraw after asynchronous events more 'naturally'. In any case this is something to be fixed in another PR

Some may be wondering why I'm using job ids instead of opaque pointers(could have made the fields of Job private to the job module for example) and the reason is simple: I couldn't find a way to pass arbitrary pointers to vimscript, the simplest solution was to use integer ids.

I think this is ready for review, but I will only merge it after #476 . After this is merged I can implement the msgpack API hopefully we'll be able to use python plugins.

Member

tarruda commented Apr 7, 2014

I'm not satisfied with having to call shell_resized after handling each job event. The problem seems to be that the whole redrawing logic is scattered across edit.c/normal.c(not sure though, I have tried playing with the functions in screen.c but couldn't find anything obvious) and is executed whenever keys are pressed. The shell_resized call can be removed by using the 'events as keys' hack I proposed in #395, this will make vim redraw after asynchronous events more 'naturally'. In any case this is something to be fixed in another PR

Some may be wondering why I'm using job ids instead of opaque pointers(could have made the fields of Job private to the job module for example) and the reason is simple: I couldn't find a way to pass arbitrary pointers to vimscript, the simplest solution was to use integer ids.

I think this is ready for review, but I will only merge it after #476 . After this is merged I can implement the msgpack API hopefully we'll be able to use python plugins.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 7, 2014

Member

@tarruda Playing around with netcat worked beautifully. One question so far: there doesn't seem to be any protection from overwriting an existing job with the same job name:

:jobstart('netcat', 'nc', ['-l', '1234'])
:jobstart('netcat', 'nc', ['-l', '1234'])

For a plugin to avoid collisions like this, it would be useful if jobstart() with, say, an empty string as the first parameter returned the next available job id and registered the job without overwriting an existing job. Would it be a bad idea for au JobActivity 1 to match job id 1? That would probably require disallowing job names starting with a number.

Member

justinmk commented Apr 7, 2014

@tarruda Playing around with netcat worked beautifully. One question so far: there doesn't seem to be any protection from overwriting an existing job with the same job name:

:jobstart('netcat', 'nc', ['-l', '1234'])
:jobstart('netcat', 'nc', ['-l', '1234'])

For a plugin to avoid collisions like this, it would be useful if jobstart() with, say, an empty string as the first parameter returned the next available job id and registered the job without overwriting an existing job. Would it be a bad idea for au JobActivity 1 to match job id 1? That would probably require disallowing job names starting with a number.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 7, 2014

Member

@justinmk they won't overwrite each other, I think what happened is that the second call to netcat failed because it tried listening on the same port as the first.

When you pass a name to jobstart you are actually saying to Neovim: 'When that job sends you some data, emit an 'event' with that name'. This is similar to the event emitter pattern in javascript, except that I tried to use the autocommand infrastructure for it. You can also listen for a namespace of events like this:

:let srv1_id = jobstart('netcat-server-1', 'nc', ['-l', '9991'])
:let srv2_id = jobstart('netcat-server-2', 'nc', ['-l', '9992'])

au JobActivity netcat-server-* call append(line('$'), 'message from server '.v:job_data[0].': '. v:job_data[1])
Member

tarruda commented Apr 7, 2014

@justinmk they won't overwrite each other, I think what happened is that the second call to netcat failed because it tried listening on the same port as the first.

When you pass a name to jobstart you are actually saying to Neovim: 'When that job sends you some data, emit an 'event' with that name'. This is similar to the event emitter pattern in javascript, except that I tried to use the autocommand infrastructure for it. You can also listen for a namespace of events like this:

:let srv1_id = jobstart('netcat-server-1', 'nc', ['-l', '9991'])
:let srv2_id = jobstart('netcat-server-2', 'nc', ['-l', '9992'])

au JobActivity netcat-server-* call append(line('$'), 'message from server '.v:job_data[0].': '. v:job_data[1])
@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 7, 2014

Member

I've updated to use the modifications sent by @philix in #476 but I dont like the way we have to forward declare the Event/Job types to avoid circular dependency problem between the headers. Perhaps we should move typedefs/structs to *_def.h files?

Member

tarruda commented Apr 7, 2014

I've updated to use the modifications sent by @philix in #476 but I dont like the way we have to forward declare the Event/Job types to avoid circular dependency problem between the headers. Perhaps we should move typedefs/structs to *_def.h files?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 7, 2014

Member

similar to the event emitter pattern in javascript, except that I tried to use the autocommand infrastructure

From a user interface perspective, I think the design works well, so far.

Member

justinmk commented Apr 7, 2014

similar to the event emitter pattern in javascript, except that I tried to use the autocommand infrastructure

From a user interface perspective, I think the design works well, so far.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 7, 2014

Member

From a user interface perspective, I think the design works well, so far.

There's still some things missing:

  • Get a list of jobs
  • Get detailed information about a job, such a pid
  • Notification about when a job exits

But I think it's better to worry about those after the plugin infrastructure is implemented because there might still be some modifications to the job module.

Member

tarruda commented Apr 7, 2014

From a user interface perspective, I think the design works well, so far.

There's still some things missing:

  • Get a list of jobs
  • Get detailed information about a job, such a pid
  • Notification about when a job exits

But I think it's better to worry about those after the plugin infrastructure is implemented because there might still be some modifications to the job module.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 7, 2014

Member

Rebased/squashed, if no one objects in one hour I will merge it

Member

tarruda commented Apr 7, 2014

Rebased/squashed, if no one objects in one hour I will merge it

Show outdated Hide outdated src/globals.h
Show outdated Hide outdated src/eval.c
Show outdated Hide outdated src/os/job.c
Show outdated Hide outdated src/os/job.c
@@ -589,6 +590,7 @@ void mch_exit(int r)
{
exiting = TRUE;
job_teardown();

This comment has been minimized.

@justinmk

justinmk Apr 7, 2014

Member

is this also needed in preserve_exit() or some part of that call chain?

@justinmk

justinmk Apr 7, 2014

Member

is this also needed in preserve_exit() or some part of that call chain?

This comment has been minimized.

@tarruda

tarruda Apr 7, 2014

Member

preserve_exit calls getout which calls mch_exit

@tarruda

tarruda Apr 7, 2014

Member

preserve_exit calls getout which calls mch_exit

Show outdated Hide outdated src/os/job.c
uv_read_start((uv_stream_t *)&job->proc_stderr, alloc_cb, read_cb);
}
static bool is_alive(Job *job)

This comment has been minimized.

@justinmk

justinmk Apr 7, 2014

Member

is_foo() functions with side effects are questionable. Maybe rename to try_kill() or something.

@justinmk

justinmk Apr 7, 2014

Member

is_foo() functions with side effects are questionable. Maybe rename to try_kill() or something.

This comment has been minimized.

@tarruda

tarruda Apr 7, 2014

Member

But it's not trying to kill or send a signal, sending 0 is just a way to see if the process is alive

@tarruda

tarruda Apr 7, 2014

Member

But it's not trying to kill or send a signal, sending 0 is just a way to see if the process is alive

event_push(event);
}
static void write_cb(uv_write_t *req, int status)

This comment has been minimized.

@justinmk

justinmk Apr 7, 2014

Member

status intentionally unused?

@justinmk

justinmk Apr 7, 2014

Member

status intentionally unused?

This comment has been minimized.

@tarruda

tarruda Apr 7, 2014

Member

status might indicate a failure, we have two choices here:

  • Notify the user in some way
  • Kill the process(not necessary if the reason for failure is because it was killed already)

I think the sane choice here is to kill the process, we don't want a job that we cant send data to, either way the user will be notified(I'm thinking of calling JobActivity with 0 or empty string is a reasonable way of notifying the job has exited)

@tarruda

tarruda Apr 7, 2014

Member

status might indicate a failure, we have two choices here:

  • Notify the user in some way
  • Kill the process(not necessary if the reason for failure is because it was killed already)

I think the sane choice here is to kill the process, we don't want a job that we cant send data to, either way the user will be notified(I'm thinking of calling JobActivity with 0 or empty string is a reasonable way of notifying the job has exited)

Show outdated Hide outdated src/os/job.c
Show outdated Hide outdated src/os/job.c
@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 7, 2014

Member

@justinmk / @oni-link Thanks for the feedback, these last commits should have fixed the issues reported

Member

tarruda commented Apr 7, 2014

@justinmk / @oni-link Thanks for the feedback, these last commits should have fixed the issues reported

Show outdated Hide outdated src/os/job.c
continue;
}
if ((job->exit_timeout--) == EXIT_TIMEOUT) {

This comment has been minimized.

@oni-link

oni-link Apr 7, 2014

Contributor

Here only one SIGTERM is ever sent, but description of exit_timeout says up to 25.

Did you mean something like this?

int signal = job->exit_timeout-- > 0 ? SIGTERM : SIGKILL;
uv_process_kill(&job->proc, signal);
@oni-link

oni-link Apr 7, 2014

Contributor

Here only one SIGTERM is ever sent, but description of exit_timeout says up to 25.

Did you mean something like this?

int signal = job->exit_timeout-- > 0 ? SIGTERM : SIGKILL;
uv_process_kill(&job->proc, signal);

This comment has been minimized.

@tarruda

tarruda Apr 7, 2014

Member

Theoretically one SIGTERM should be enough, so the comment was wrong. The timeout represents the number of polls that will be done before actually sending SIGKILL

@tarruda

tarruda Apr 7, 2014

Member

Theoretically one SIGTERM should be enough, so the comment was wrong. The timeout represents the number of polls that will be done before actually sending SIGKILL

Implement job control
- Add a job control module for spawning and controlling co-processes
- Add three vimscript functions for interfacing with the module
- Use dedicated header files for typedefs/structs in event/job modules
uv_close((uv_handle_t *)&job->proc_stdin, NULL);
uv_close((uv_handle_t *)&job->proc_stderr, NULL);
uv_close((uv_handle_t *)&job->proc, NULL);
free(job);

This comment has been minimized.

@oni-link

oni-link Apr 7, 2014

Contributor

Is it save to free job at this point? Are all callbacks that use job canceled (synchronously), or are they waiting for one last run of the event loop?

@oni-link

oni-link Apr 7, 2014

Contributor

Is it save to free job at this point? Are all callbacks that use job canceled (synchronously), or are they waiting for one last run of the event loop?

This comment has been minimized.

@tarruda

tarruda Apr 7, 2014

Member

As far as I know it is safe. Calling uv_close should remove any references from the event loop so no callbacks will be run after this.

@tarruda

tarruda Apr 7, 2014

Member

As far as I know it is safe. Calling uv_close should remove any references from the event loop so no callbacks will be run after this.

@tarruda tarruda merged commit 4b063ea into neovim:master Apr 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
kBufferLockStderr ///< Data read from stderr
} BufferLock;
struct _Job {

This comment has been minimized.

@philix

philix Apr 8, 2014

Member

Per the style-guide it should be struct job. If job conflicts with something then you could call it job_s or job_struct. Consistent rules for type names is the most useful thing that comes from a coding style guide IMO.

@philix

philix Apr 8, 2014

Member

Per the style-guide it should be struct job. If job conflicts with something then you could call it job_s or job_struct. Consistent rules for type names is the most useful thing that comes from a coding style guide IMO.

This comment has been minimized.

@tarruda

tarruda Apr 8, 2014

Member

Missed that, sorry

@tarruda

tarruda Apr 8, 2014

Member

Missed that, sorry

{
uv_disable_stdio_inheritance();
uv_prepare_init(uv_default_loop(), &job_prepare);
uv_prepare_start(&job_prepare, job_prepare_cb);

This comment has been minimized.

@oni-link

oni-link Apr 18, 2014

Contributor

@tarruda Why not call uv_prepare_start when the first job is started and call uv_prepare_stop after the job table is empty? Otherwise the job_prepare_cb is called every iteration of the uv loop.

@oni-link

oni-link Apr 18, 2014

Contributor

@tarruda Why not call uv_prepare_start when the first job is started and call uv_prepare_stop after the job table is empty? Otherwise the job_prepare_cb is called every iteration of the uv loop.

This comment has been minimized.

@tarruda

tarruda Apr 18, 2014

Member

👍

@tarruda
return;
}
// Cleanup argv memory in case the `job_start` call failed
shell_free_argv(argv);

This comment has been minimized.

@oni-link

oni-link Apr 18, 2014

Contributor

@tarruda Why not move this call and the call shell_free_argv from job_exit_event to close_cb in job.c. Then the job module can always take care of the free and the whole cleanup:-block can be removed (also replace goto cleanup;, see above).

@oni-link

oni-link Apr 18, 2014

Contributor

@tarruda Why not move this call and the call shell_free_argv from job_exit_event to close_cb in job.c. Then the job module can always take care of the free and the whole cleanup:-block can be removed (also replace goto cleanup;, see above).

This comment has been minimized.

@tarruda

tarruda Apr 18, 2014

Member

👍

@tarruda

This comment has been minimized.

@tarruda

tarruda Apr 18, 2014

Member

Not sure about this, the cleanup label exists for handling errors detected in this function, before job_start is even called.

@tarruda

tarruda Apr 18, 2014

Member

Not sure about this, the cleanup label exists for handling errors detected in this function, before job_start is even called.

This comment has been minimized.

@oni-link

oni-link Apr 18, 2014

Contributor

Not sure if I have understood your concerns correctly:

There is no error handling really, the function just returns, if any goto cleanup; is reached:
When jumped to this label rettv->vval.v_number is 0 and argv is NULL, always. A call to shell_free_argv returns immediatley, without freeing anything. So the cleanup does nothing.

The only purpose of this label is to free argv if job_start failed, which could be done more consitent in the job module.

@oni-link

oni-link Apr 18, 2014

Contributor

Not sure if I have understood your concerns correctly:

There is no error handling really, the function just returns, if any goto cleanup; is reached:
When jumped to this label rettv->vval.v_number is 0 and argv is NULL, always. A call to shell_free_argv returns immediatley, without freeing anything. So the cleanup does nothing.

The only purpose of this label is to free argv if job_start failed, which could be done more consitent in the job module.

This comment has been minimized.

@tarruda

tarruda Apr 18, 2014

Member

You are right, the label is useless. I will push the fix to #556

@tarruda

tarruda Apr 18, 2014

Member

You are right, the label is useless. I will push the fix to #556

argv[i] = NULL;
rettv->vval.v_number = job_start(argv,
xstrdup((char *)argvars[0].vval.v_string),

This comment has been minimized.

@oni-link

oni-link Apr 18, 2014

Contributor

@tarruda Could not find a free for this pointer. Look like a job for close_cb in job.c.

@oni-link

oni-link Apr 18, 2014

Contributor

@tarruda Could not find a free for this pointer. Look like a job for close_cb in job.c.

This comment has been minimized.

@tarruda

tarruda Apr 18, 2014

Member

👍

thanks for having the patience to read the code and point errors/improvements. I'm pushing fixes to #556

@tarruda

tarruda Apr 18, 2014

Member

👍

thanks for having the patience to read the code and point errors/improvements. I'm pushing fixes to #556

Grimy pushed a commit to Grimy/neovim that referenced this pull request Jan 7, 2015

Merge pull request #475 from jmesmon/fix474
Use debian6 package for 32bit llvm+clang, ubuntu one is actually 64bit

@LeifW LeifW referenced this pull request Mar 3, 2015

Closed

Idris not started? #34

dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment