# HG changeset patch # User Christian Brabandt # Date 1479400204 -3600 # Node ID d3f0946b4a8058d2b96a4575878082ec04e5e12e # Parent 368468ef35cfbe0646633091fe4d8fae5ec66c3e commit https://github.com/vim/vim/commit/7df915d113ac1981792c50e8b000c9f5f784b78b Author: Bram Moolenaar Date: Thu Nov 17 17:25:32 2016 +0100 patch 8.0.0087 Problem: When the channel callback gets job info the job may already have been deleted. (lifepillar) Solution: Do not delete the job when the channel is still useful. (ichizok, closes #1242, closes #1245) diff --git a/src/channel.c b/src/channel.c --- a/src/channel.c +++ b/src/channel.c @@ -4433,19 +4433,66 @@ job_free(job_T *job) } } +#if defined(EXITFREE) || defined(PROTO) + void +job_free_all(void) +{ + while (first_job != NULL) + job_free(first_job); +} +#endif + +/* + * Return TRUE if we need to check if the process of "job" has ended. + */ + static int +job_need_end_check(job_T *job) +{ + return job->jv_status == JOB_STARTED + && (job->jv_stoponexit != NULL || job->jv_exit_cb != NULL); +} + +/* + * Return TRUE if the channel of "job" is still useful. + */ + static int +job_channel_still_useful(job_T *job) +{ + return job->jv_channel != NULL && channel_still_useful(job->jv_channel); +} + +/* + * Return TRUE if the job should not be freed yet. Do not free the job when + * it has not ended yet and there is a "stoponexit" flag, an exit callback + * or when the associated channel will do something with the job output. + */ + static int +job_still_useful(job_T *job) +{ + return job_need_end_check(job) || job_channel_still_useful(job); +} + +/* + * NOTE: Must call job_cleanup() only once right after the status of "job" + * changed to JOB_ENDED (i.e. after job_status() returned "dead" first or + * mch_detect_ended_job() returned non-NULL). + */ static void job_cleanup(job_T *job) { if (job->jv_status != JOB_ENDED) return; + /* Ready to cleanup the job. */ + job->jv_status = JOB_FINISHED; + if (job->jv_exit_cb != NULL) { typval_T argv[3]; typval_T rettv; int dummy; - /* invoke the exit callback; make sure the refcount is > 0 */ + /* Invoke the exit callback. Make sure the refcount is > 0. */ ++job->jv_refcount; argv[0].v_type = VAR_JOB; argv[0].vval.v_job = job; @@ -4458,42 +4505,18 @@ job_cleanup(job_T *job) --job->jv_refcount; channel_need_redraw = TRUE; } - if (job->jv_refcount == 0) + + /* Do not free the job in case the close callback of the associated channel + * isn't invoked yet and may get information by job_info(). */ + if (job->jv_refcount == 0 && !job_channel_still_useful(job)) { - /* The job was already unreferenced, now that it ended it can be - * freed. Careful: caller must not use "job" after this! */ + /* The job was already unreferenced and the associated channel was + * detached, now that it ended it can be freed. Careful: caller must + * not use "job" after this! */ job_free(job); } } -#if defined(EXITFREE) || defined(PROTO) - void -job_free_all(void) -{ - while (first_job != NULL) - job_free(first_job); -} -#endif - -/* - * Return TRUE if the job should not be freed yet. Do not free the job when - * it has not ended yet and there is a "stoponexit" flag, an exit callback - * or when the associated channel will do something with the job output. - */ - static int -job_still_useful(job_T *job) -{ - return (job->jv_stoponexit != NULL || job->jv_exit_cb != NULL - || (job->jv_channel != NULL - && channel_still_useful(job->jv_channel))); -} - - static int -job_still_alive(job_T *job) -{ - return (job->jv_status == JOB_STARTED) && job_still_useful(job); -} - /* * Mark references in jobs that are still useful. */ @@ -4505,7 +4528,7 @@ set_ref_in_job(int copyID) typval_T tv; for (job = first_job; job != NULL; job = job->jv_next) - if (job_still_alive(job)) + if (job_still_useful(job)) { tv.v_type = VAR_JOB; tv.vval.v_job = job; @@ -4514,26 +4537,33 @@ set_ref_in_job(int copyID) return abort; } +/* + * Dereference "job". Note that after this "job" may have been freed. + */ void job_unref(job_T *job) { if (job != NULL && --job->jv_refcount <= 0) { - /* Do not free the job when it has not ended yet and there is a - * "stoponexit" flag or an exit callback. */ - if (!job_still_alive(job)) + /* Do not free the job if there is a channel where the close callback + * may get the job info. */ + if (!job_channel_still_useful(job)) { - job_free(job); - } - else if (job->jv_channel != NULL - && !channel_still_useful(job->jv_channel)) - { - /* Do remove the link to the channel, otherwise it hangs - * around until Vim exits. See job_free() for refcount. */ - ch_log(job->jv_channel, "detaching channel from job"); - job->jv_channel->ch_job = NULL; - channel_unref(job->jv_channel); - job->jv_channel = NULL; + /* Do not free the job when it has not ended yet and there is a + * "stoponexit" flag or an exit callback. */ + if (!job_need_end_check(job)) + { + job_free(job); + } + else if (job->jv_channel != NULL) + { + /* Do remove the link to the channel, otherwise it hangs + * around until Vim exits. See job_free() for refcount. */ + ch_log(job->jv_channel, "detaching channel from job"); + job->jv_channel->ch_job = NULL; + channel_unref(job->jv_channel); + job->jv_channel = NULL; + } } } } @@ -4546,7 +4576,7 @@ free_unused_jobs_contents(int copyID, in for (job = first_job; job != NULL; job = job->jv_next) if ((job->jv_copyID & mask) != (copyID & mask) - && !job_still_alive(job)) + && !job_still_useful(job)) { /* Free the channel and ordinary items it contains, but don't * recurse into Lists, Dictionaries etc. */ @@ -4566,7 +4596,7 @@ free_unused_jobs(int copyID, int mask) { job_next = job->jv_next; if ((job->jv_copyID & mask) != (copyID & mask) - && !job_still_alive(job)) + && !job_still_useful(job)) { /* Free the job struct itself. */ job_free_job(job); @@ -4660,8 +4690,7 @@ has_pending_job(void) /* Only should check if the channel has been closed, if the channel is * open the job won't exit. */ if (job->jv_status == JOB_STARTED && job->jv_exit_cb != NULL - && (job->jv_channel == NULL - || !channel_still_useful(job->jv_channel))) + && !job_channel_still_useful(job)) return TRUE; return FALSE; } @@ -4676,14 +4705,18 @@ job_check_ended(void) { int i; + if (first_job == NULL) + return; + for (i = 0; i < MAX_CHECK_ENDED; ++i) { + /* NOTE: mch_detect_ended_job() must only return a job of which the + * status was just set to JOB_ENDED. */ job_T *job = mch_detect_ended_job(first_job); if (job == NULL) break; - if (job_still_useful(job)) - job_cleanup(job); /* may free "job" */ + job_cleanup(job); /* may free "job" */ } if (channel_need_redraw) @@ -4897,7 +4930,7 @@ job_status(job_T *job) { char *result; - if (job->jv_status == JOB_ENDED) + if (job->jv_status >= JOB_ENDED) /* No need to check, dead is dead. */ result = "dead"; else if (job->jv_status == JOB_FAILED) diff --git a/src/eval.c b/src/eval.c --- a/src/eval.c +++ b/src/eval.c @@ -7290,7 +7290,7 @@ get_tv_string_buf_chk(typval_T *varp, ch if (job == NULL) return (char_u *)"no process"; status = job->jv_status == JOB_FAILED ? "fail" - : job->jv_status == JOB_ENDED ? "dead" + : job->jv_status >= JOB_ENDED ? "dead" : "run"; # ifdef UNIX vim_snprintf((char *)buf, NUMBUFLEN, diff --git a/src/os_unix.c b/src/os_unix.c --- a/src/os_unix.c +++ b/src/os_unix.c @@ -5354,7 +5354,7 @@ mch_job_status(job_T *job) return "run"; return_dead: - if (job->jv_status != JOB_ENDED) + if (job->jv_status < JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; @@ -5398,7 +5398,7 @@ mch_detect_ended_job(job_T *job_list) job->jv_exitval = WEXITSTATUS(status); else if (WIFSIGNALED(status)) job->jv_exitval = -1; - if (job->jv_status != JOB_ENDED) + if (job->jv_status < JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; diff --git a/src/os_win32.c b/src/os_win32.c --- a/src/os_win32.c +++ b/src/os_win32.c @@ -4978,7 +4978,7 @@ mch_job_status(job_T *job) || dwExitCode != STILL_ACTIVE) { job->jv_exitval = (int)dwExitCode; - if (job->jv_status != JOB_ENDED) + if (job->jv_status < JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; diff --git a/src/structs.h b/src/structs.h --- a/src/structs.h +++ b/src/structs.h @@ -1421,11 +1421,13 @@ struct partial_S dict_T *pt_dict; /* dict for "self" */ }; +/* Status of a job. Order matters! */ typedef enum { JOB_FAILED, JOB_STARTED, - JOB_ENDED + JOB_ENDED, /* detected job done */ + JOB_FINISHED /* job done and cleanup done */ } jobstatus_T; /* diff --git a/src/testdir/test_channel.vim b/src/testdir/test_channel.vim --- a/src/testdir/test_channel.vim +++ b/src/testdir/test_channel.vim @@ -1232,6 +1232,32 @@ func Test_out_cb_lambda() endtry endfunc +func Test_close_and_exit_cb() + if !has('job') + return + endif + call ch_log('Test_close_and_exit_cb') + + let dict = {'ret': {}} + func dict.close_cb(ch) dict + let self.ret['close_cb'] = job_status(ch_getjob(a:ch)) + endfunc + func dict.exit_cb(job, status) dict + let self.ret['exit_cb'] = job_status(a:job) + endfunc + + let g:job = job_start('echo', { + \ 'close_cb': dict.close_cb, + \ 'exit_cb': dict.exit_cb, + \ }) + call assert_equal('run', job_status(g:job)) + unlet g:job + call WaitFor('len(dict.ret) >= 2') + call assert_equal(2, len(dict.ret)) + call assert_match('^\%(dead\|run\)', dict.ret['close_cb']) + call assert_equal('dead', dict.ret['exit_cb']) +endfunc + """""""""" let g:Ch_unletResponse = '' diff --git a/src/version.c b/src/version.c --- a/src/version.c +++ b/src/version.c @@ -765,6 +765,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ /**/ + 87, +/**/ 86, /**/ 85,