To: vim_dev@googlegroups.com
Subject: Patch 7.4.1447
Fcc: outbox
From: Bram Moolenaar <Bram@moolenaar.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
------------

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.
Files:      src/eval.c, src/channel.c


*** ../vim-7.4.1446/src/eval.c	2016-02-27 21:27:16.550519087 +0100
--- src/eval.c	2016-02-28 15:28:31.019487430 +0100
***************
*** 7741,7747 ****
  /*
   * 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.
   */
      int
  channel_unref(channel_T *channel)
--- 7741,7747 ----
  /*
   * 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 is no longer referenced.
   */
      int
  channel_unref(channel_T *channel)
***************
*** 7762,7767 ****
--- 7762,7768 ----
  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,7802 ****
--- 7797,7813 ----
  	 * "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,10473 ****
--- 10478,10487 ----
  		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,15297 ****
--- 15306,15314 ----
  		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
*** ../vim-7.4.1446/src/channel.c	2016-02-27 21:52:56.622336150 +0100
--- src/channel.c	2016-02-28 15:34:49.323512394 +0100
***************
*** 311,318 ****
--- 311,321 ----
  }
  
  /*
+  * 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,352 ****
--- 350,356 ----
  {
      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,780 ****
--- 779,788 ----
  }
  #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,1427 ****
--- 1429,1438 ----
  	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,1481 ****
  	{
  	    if (item->cq_seq_nr == seq_nr)
  	    {
! 		ch_log(channel, "Invoking one-time callback");
  		/* Remove the item from the list first, if the callback
  		 * invokes ch_close() the list will be cleared. */
  		remove_cb_node(head, item);
--- 1486,1493 ----
  	{
  	    if (item->cq_seq_nr == seq_nr)
  	    {
! 		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,1494 ****
  	    item = item->cq_next;
  	}
  	if (!done)
! 	    ch_log(channel, "Dropping message without callback");
      }
      else if (callback != NULL || buffer != NULL)
      {
--- 1500,1506 ----
  	    item = item->cq_next;
  	}
  	if (!done)
! 	    ch_logn(channel, "Dropping message %d without callback", seq_nr);
      }
      else if (callback != NULL || buffer != NULL)
      {
***************
*** 1539,1544 ****
--- 1551,1558 ----
  	    invoke_callback(channel, callback, argv);
  	}
      }
+     else if (msg != NULL)
+ 	ch_logs(channel, "Dropping message '%s'", (char *)msg);
      else
  	ch_log(channel, "Dropping message");
  
***************
*** 1637,1642 ****
--- 1651,1658 ----
  
  	  /* 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,1709 ****
--- 1720,1726 ----
      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,1726 ****
--- 1738,1744 ----
  {
      channel_T *channel;
  
+     ch_log(NULL, "channel_free_all()");
      for (channel = first_channel; channel != NULL; channel = channel->ch_next)
  	channel_clear(channel);
  }
***************
*** 1798,1804 ****
  	    return OK;
  #endif
      }
-     ch_log(channel, "Nothing to read");
      return FAIL;
  }
  
--- 1816,1821 ----
***************
*** 1970,1980 ****
   */
      int
  channel_read_json_block(
! 	channel_T *channel,
! 	int part,
! 	int timeout,
! 	int id,
! 	typval_T **rettv)
  {
      int		more;
      sock_T	fd;
--- 1987,1997 ----
   */
      int
  channel_read_json_block(
! 	channel_T   *channel,
! 	int	    part,
! 	int	    timeout,
! 	int	    id,
! 	typval_T    **rettv)
  {
      int		more;
      sock_T	fd;
*** ../vim-7.4.1446/src/version.c	2016-02-28 15:21:10.004115041 +0100
--- src/version.c	2016-02-28 15:23:19.446764741 +0100
***************
*** 745,746 ****
--- 745,748 ----
  {   /* Add new patch number below this line */
+ /**/
+     1447,
  /**/

-- 
TERRY GILLIAM PLAYED: PATSY (ARTHUR'S TRUSTY STEED), THE GREEN KNIGHT
                      SOOTHSAYER, BRIDGEKEEPER, SIR GAWAIN (THE FIRST TO BE
                      KILLED BY THE RABBIT)
                 "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

 /// 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    ///