patch 7.4.1447

Problem:    Memory leak when using ch_read(). (Dominique Pelle)
            No log message when stopping a job and a few other situations.
            Too many "Nothing to read" messages.  Channels are not freed.
Solution:   Free the listtv.  Add more log messages. Remove "Nothing to read"
            message.  Remove the channel from the job when its refcount
            becomes zero.

diff --git a/src/channel.c b/src/channel.c
index da76cbe82a6e0616f0f81288a53a49b72292a528..c6f94ee2b9dc9e8284da18f24f1c36599eab2b22 100644 (file)
--- a/src/channel.c
+++ b/src/channel.c
@@ -311,8 +311,11 @@ add_channel(void)
 }
 
 /*
+ * Called when the refcount of a channel is zero.
  * Return TRUE if "channel" has a callback and the associated job wasn't
  * killed.
+ * If the job was killed the channel is not expected to work anymore.
+ * If there is no callback then nobody can get readahead.
  */
     static int
 channel_still_useful(channel_T *channel)
@@ -347,6 +350,7 @@ channel_free(channel_T *channel)
 {
     channel_close(channel, TRUE);
     channel_clear(channel);
+    ch_log(channel, "Freeing channel");
     if (channel->ch_next != NULL)
        channel->ch_next->ch_prev = channel->ch_prev;
     if (channel->ch_prev == NULL)
@@ -775,6 +779,10 @@ channel_set_pipes(channel_T *channel, sock_T in, sock_T out, sock_T err)
 }
 #endif
 
+/*
+ * Sets the job the channel is associated with.
+ * This does not keep a refcount, when the job is freed ch_job is cleared.
+ */
     void
 channel_set_job(channel_T *channel, job_T *job)
 {
@@ -1421,7 +1429,10 @@ may_invoke_callback(channel_T *channel, int part)
        if (callback == NULL && buffer == NULL)
        {
            while ((msg = channel_get(channel, part)) != NULL)
+           {
+               ch_logs(channel, "Dropping message '%s'", (char *)msg);
                vim_free(msg);
+           }
            return FALSE;
        }
 
@@ -1475,7 +1486,8 @@ may_invoke_callback(channel_T *channel, int part)
        {
            if (item->cq_seq_nr == seq_nr)
            {
-               ch_log(channel, "Invoking one-time callback");
+               ch_logs(channel, "Invoking one-time callback '%s'",
+                                                  (char *)item->cq_callback);
                /* Remove the item from the list first, if the callback
                 * invokes ch_close() the list will be cleared. */
                remove_cb_node(head, item);
@@ -1488,7 +1500,7 @@ may_invoke_callback(channel_T *channel, int part)
            item = item->cq_next;
        }
        if (!done)
-           ch_log(channel, "Dropping message without callback");
+           ch_logn(channel, "Dropping message %d without callback", seq_nr);
     }
     else if (callback != NULL || buffer != NULL)
     {
@@ -1539,6 +1551,8 @@ may_invoke_callback(channel_T *channel, int part)
            invoke_callback(channel, callback, argv);
        }
     }
+    else if (msg != NULL)
+       ch_logs(channel, "Dropping message '%s'", (char *)msg);
     else
        ch_log(channel, "Dropping message");
 
@@ -1637,6 +1651,8 @@ channel_close(channel_T *channel, int invoke_close_cb)
 
          /* invoke the close callback; increment the refcount to avoid it
           * being freed halfway */
+         ch_logs(channel, "Invoking close callback %s",
+                                               (char *)channel->ch_close_cb);
          argv[0].v_type = VAR_CHANNEL;
          argv[0].vval.v_channel = channel;
          ++channel->ch_refcount;
@@ -1704,6 +1720,7 @@ channel_clear_one(channel_T *channel, int part)
     void
 channel_clear(channel_T *channel)
 {
+    ch_log(channel, "Clearing channel");
     channel_clear_one(channel, PART_SOCK);
 #ifdef CHANNEL_PIPES
     channel_clear_one(channel, PART_OUT);
@@ -1721,6 +1738,7 @@ channel_free_all(void)
 {
     channel_T *channel;
 
+    ch_log(NULL, "channel_free_all()");
     for (channel = first_channel; channel != NULL; channel = channel->ch_next)
        channel_clear(channel);
 }
@@ -1798,7 +1816,6 @@ channel_wait(channel_T *channel, sock_T fd, int timeout)
            return OK;
 #endif
     }
-    ch_log(channel, "Nothing to read");
     return FAIL;
 }
 
@@ -1970,11 +1987,11 @@ channel_read_block(channel_T *channel, int part, int timeout)
  */
     int
 channel_read_json_block(
-       channel_T *channel,
-       int part,
-       int timeout,
-       int id,
-       typval_T **rettv)
+       channel_T   *channel,
+       int         part,
+       int         timeout,
+       int         id,
+       typval_T    **rettv)
 {
     int                more;
     sock_T     fd;

diff --git a/src/eval.c b/src/eval.c
index 4afceccfaf12c9f0fbdd29c9a49cde8f6c6d344a..c12961aa78cc46d761b3634151edc76a73ba6078 100644 (file)
--- a/src/eval.c
+++ b/src/eval.c
@@ -7741,7 +7741,7 @@ failret:
 /*
  * Decrement the reference count on "channel" and maybe free it when it goes
  * down to zero.  Don't free it if there is a pending action.
- * Returns TRUE when the channel was freed.
+ * Returns TRUE when the channel is no longer referenced.
  */
     int
 channel_unref(channel_T *channel)
@@ -7762,6 +7762,7 @@ static job_T *first_job = NULL;
 job_free(job_T *job)
 {
 # ifdef FEAT_CHANNEL
+    ch_log(job->jv_channel, "Freeing job");
     if (job->jv_channel != NULL)
     {
        /* The link from the channel to the job doesn't count as a reference,
@@ -7796,7 +7797,17 @@ job_unref(job_T *job)
         * "stoponexit" flag or an exit callback. */
        if (job->jv_status != JOB_STARTED
                || (job->jv_stoponexit == NULL && job->jv_exit_cb == NULL))
+       {
            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. */
+           job->jv_channel->ch_job = NULL;
+           channel_unref(job->jv_channel);
+           job->jv_channel = NULL;
+       }
     }
 }
 
@@ -10467,7 +10478,10 @@ common_channel_read(typval_T *argvars, typval_T *rettv, int raw)
                id = opt.jo_id;
            channel_read_json_block(channel, part, timeout, id, &listtv);
            if (listtv != NULL)
+           {
                *rettv = *listtv;
+               vim_free(listtv);
+           }
            else
            {
                rettv->v_type = VAR_SPECIAL;
@@ -15292,6 +15306,9 @@ f_job_stop(typval_T *argvars UNUSED, typval_T *rettv UNUSED)
                return;
            }
        }
+# ifdef FEAT_CHANNEL
+       ch_logs(job->jv_channel, "Stopping job with '%s'", (char *)arg);
+# endif
        if (mch_stop_job(job, arg) == FAIL)
            rettv->vval.v_number = 0;
        else

diff --git a/src/version.c b/src/version.c
index d13cfb7da804ee1be0bb330778e58e3c7d562b49..f3a9183b68d1af80a2612224058f06b6e47fe700 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -743,6 +743,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1447,
 /**/
     1446,
 /**/