Permalink
Browse files

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)
  • Loading branch information...
1 parent c0514bf commit 7df915d113ac1981792c50e8b000c9f5f784b78b @brammool brammool committed Nov 17, 2016
Showing with 122 additions and 59 deletions.
  1. +87 −54 src/channel.c
  2. +1 −1 src/eval.c
  3. +2 −2 src/os_unix.c
  4. +1 −1 src/os_win32.c
  5. +3 −1 src/structs.h
  6. +26 −0 src/testdir/test_channel.vim
  7. +2 −0 src/version.c
View
@@ -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, int mask)
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)
View
@@ -7290,7 +7290,7 @@ get_tv_string_buf_chk(typval_T *varp, char_u *buf)
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,
View
@@ -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;
View
@@ -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;
View
@@ -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;
/*
@@ -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 = ''
View
@@ -765,6 +765,8 @@ static char *(features[]) =
static int included_patches[] =
{ /* Add new patch number below this line */
/**/
+ 87,
+/**/
86,
/**/
85,

0 comments on commit 7df915d

Please sign in to comment.