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
index 1ddb1ece35ccf4df922c6caf072502a9bfed4b27..778a30e17c552a6da458999c54a2e83ca07ad453 100644 (file)
--- 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, 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)

diff --git a/src/eval.c b/src/eval.c
index 7ca129b378833486348b27184345040c1dbda948..9d2c5ac72b3a09b5befe1618b7f03bc07c5c4892 100644 (file)
--- a/src/eval.c
+++ b/src/eval.c
@@ -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,

diff --git a/src/os_unix.c b/src/os_unix.c
index 6197c3fbddb8542011fc424c2ebd3631c061105b..a63eb6e89196e8b22798c8d4a0958abc8e8c8e99 100644 (file)
--- 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
index 9fcb054d0e9a0c1b46f97599dbaf656f41361ab8..f8e0f117d74b509e46b38aaac692a85e99c0ef33 100644 (file)
--- 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
index 31accea0509ba46ead69cd0ed464d6de244d86ec..c749a3691dde14ed540c014cb5918838a699644b 100644 (file)
--- 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
index c21e617b1e362bd43d79fd6845e1a9faacaec200..42e0e04b8176f3623d70c7db2411c8713b6c4f0b 100644 (file)
--- 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
index b1bf796dedb62319eb74296da60503a546235362..074c7ebc76ab03f5f00de85f5c404d1be461d370 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -764,6 +764,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    87,
 /**/
     86,
 /**/