To: vim_dev@googlegroups.com Subject: Patch 8.0.0087 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ 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) Files: src/channel.c, src/eval.c, src/os_unix.c, src/os_win32.c, src/structs.h, src/testdir/test_channel.vim *** ../vim-8.0.0086/src/channel.c 2016-11-12 21:12:48.538182233 +0100 --- src/channel.c 2016-11-17 17:13:15.080933875 +0100 *************** *** 4433,4451 **** } } static void job_cleanup(job_T *job) { if (job->jv_status != JOB_ENDED) return; 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 */ ++job->jv_refcount; argv[0].v_type = VAR_JOB; argv[0].vval.v_job = job; --- 4433,4498 ---- } } + #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. */ ++job->jv_refcount; argv[0].v_type = VAR_JOB; argv[0].vval.v_job = job; *************** *** 4458,4499 **** --job->jv_refcount; channel_need_redraw = TRUE; } ! if (job->jv_refcount == 0) ! { ! /* The job was already unreferenced, 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,4522 ---- --job->jv_refcount; channel_need_redraw = TRUE; } ! ! /* 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 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); } } /* * Mark references in jobs that are still useful. */ *************** *** 4505,4511 **** typval_T tv; for (job = first_job; job != NULL; job = job->jv_next) ! if (job_still_alive(job)) { tv.v_type = VAR_JOB; tv.vval.v_job = job; --- 4528,4534 ---- typval_T tv; for (job = first_job; job != NULL; job = job->jv_next) ! if (job_still_useful(job)) { tv.v_type = VAR_JOB; tv.vval.v_job = job; *************** *** 4514,4539 **** return abort; } 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)) ! { ! 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; } } } --- 4537,4569 ---- 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 if there is a channel where the close callback ! * may get the job info. */ ! if (!job_channel_still_useful(job)) ! { ! /* 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,4552 **** for (job = first_job; job != NULL; job = job->jv_next) if ((job->jv_copyID & mask) != (copyID & mask) ! && !job_still_alive(job)) { /* Free the channel and ordinary items it contains, but don't * recurse into Lists, Dictionaries etc. */ --- 4576,4582 ---- for (job = first_job; job != NULL; job = job->jv_next) if ((job->jv_copyID & mask) != (copyID & mask) ! && !job_still_useful(job)) { /* Free the channel and ordinary items it contains, but don't * recurse into Lists, Dictionaries etc. */ *************** *** 4566,4572 **** { job_next = job->jv_next; if ((job->jv_copyID & mask) != (copyID & mask) ! && !job_still_alive(job)) { /* Free the job struct itself. */ job_free_job(job); --- 4596,4602 ---- { job_next = job->jv_next; if ((job->jv_copyID & mask) != (copyID & mask) ! && !job_still_useful(job)) { /* Free the job struct itself. */ job_free_job(job); *************** *** 4660,4667 **** /* 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))) return TRUE; return FALSE; } --- 4690,4696 ---- /* 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_channel_still_useful(job)) return TRUE; return FALSE; } *************** *** 4676,4689 **** { int i; for (i = 0; i < MAX_CHECK_ENDED; ++i) { job_T *job = mch_detect_ended_job(first_job); if (job == NULL) break; ! if (job_still_useful(job)) ! job_cleanup(job); /* may free "job" */ } if (channel_need_redraw) --- 4705,4722 ---- { 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; ! job_cleanup(job); /* may free "job" */ } if (channel_need_redraw) *************** *** 4897,4903 **** { char *result; ! if (job->jv_status == JOB_ENDED) /* No need to check, dead is dead. */ result = "dead"; else if (job->jv_status == JOB_FAILED) --- 4930,4936 ---- { char *result; ! if (job->jv_status >= JOB_ENDED) /* No need to check, dead is dead. */ result = "dead"; else if (job->jv_status == JOB_FAILED) *** ../vim-8.0.0086/src/eval.c 2016-11-14 21:49:57.080090226 +0100 --- src/eval.c 2016-11-17 16:09:44.478455636 +0100 *************** *** 7290,7296 **** if (job == NULL) return (char_u *)"no process"; status = job->jv_status == JOB_FAILED ? "fail" ! : job->jv_status == JOB_ENDED ? "dead" : "run"; # ifdef UNIX vim_snprintf((char *)buf, NUMBUFLEN, --- 7290,7296 ---- if (job == NULL) return (char_u *)"no process"; status = job->jv_status == JOB_FAILED ? "fail" ! : job->jv_status >= JOB_ENDED ? "dead" : "run"; # ifdef UNIX vim_snprintf((char *)buf, NUMBUFLEN, *** ../vim-8.0.0086/src/os_unix.c 2016-11-12 21:12:48.538182233 +0100 --- src/os_unix.c 2016-11-17 16:09:44.478455636 +0100 *************** *** 5354,5360 **** return "run"; return_dead: ! if (job->jv_status != JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; --- 5354,5360 ---- return "run"; return_dead: ! if (job->jv_status < JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; *************** *** 5398,5404 **** job->jv_exitval = WEXITSTATUS(status); else if (WIFSIGNALED(status)) job->jv_exitval = -1; ! if (job->jv_status != JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; --- 5398,5404 ---- job->jv_exitval = WEXITSTATUS(status); else if (WIFSIGNALED(status)) job->jv_exitval = -1; ! if (job->jv_status < JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; *** ../vim-8.0.0086/src/os_win32.c 2016-10-29 14:54:56.628135821 +0200 --- src/os_win32.c 2016-11-17 16:09:44.478455636 +0100 *************** *** 4978,4984 **** || dwExitCode != STILL_ACTIVE) { job->jv_exitval = (int)dwExitCode; ! if (job->jv_status != JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; --- 4978,4984 ---- || dwExitCode != STILL_ACTIVE) { job->jv_exitval = (int)dwExitCode; ! if (job->jv_status < JOB_ENDED) { ch_log(job->jv_channel, "Job ended"); job->jv_status = JOB_ENDED; *** ../vim-8.0.0086/src/structs.h 2016-11-10 20:20:01.870602701 +0100 --- src/structs.h 2016-11-17 16:17:49.727271673 +0100 *************** *** 1421,1431 **** dict_T *pt_dict; /* dict for "self" */ }; typedef enum { JOB_FAILED, JOB_STARTED, ! JOB_ENDED } jobstatus_T; /* --- 1421,1433 ---- dict_T *pt_dict; /* dict for "self" */ }; + /* Status of a job. Order matters! */ typedef enum { JOB_FAILED, JOB_STARTED, ! JOB_ENDED, /* detected job done */ ! JOB_FINISHED /* job done and cleanup done */ } jobstatus_T; /* *** ../vim-8.0.0086/src/testdir/test_channel.vim 2016-10-27 20:00:03.665357405 +0200 --- src/testdir/test_channel.vim 2016-11-17 16:07:29.307354887 +0100 *************** *** 1232,1237 **** --- 1232,1263 ---- 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 = '' *** ../vim-8.0.0086/src/version.c 2016-11-15 21:16:46.754453019 +0100 --- src/version.c 2016-11-17 16:10:21.326213761 +0100 *************** *** 766,767 **** --- 766,769 ---- { /* Add new patch number below this line */ + /**/ + 87, /**/ -- The technology involved in making anything invisible is so infinitely complex that nine hundred and ninety-nine billion, nine hundred and ninety-nine million, nine hundred and ninety-nine thousand, nine hundred and ninety-nine times out of a trillion it is much simpler and more effective just to take the thing away and do without it. -- Douglas Adams, "The Hitchhiker's Guide to the Galaxy" /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///