leak in make-network-process

leak in make-network-process

[Dan Nicolaescu]

Do 

emacs -Q --daemon
(to make sure there's a  server running)

then 

emacs -Q -l test.el 


cat test.el

(defun gc-output (arg)
  (interactive "M")
  (with-current-buffer "*scratch*"
    (insert (format "%s %S\n" arg
		    (garbage-collect)))))

(defun server-running-p (&optional name)
  "Test whether server NAME is running.

Return values:
  nil              the server is definitely not running.
  t                the server seems to be running.
  something else   we cannot determine whether it's running without using
                   commands which may have to wait for a long time."
  (interactive
   (list (if current-prefix-arg
	     (read-string "Server name: " nil nil server-name))))
  (unless name (setq name server-name))
  (condition-case nil
      (if server-use-tcp
	  (with-temp-buffer
	    (insert-file-contents-literally (expand-file-name name server-auth-dir))
	    (or (and (looking-at "127\.0\.0\.1:[0-9]+ \\([0-9]+\\)")
		     (assq 'comm
			   (system-process-attributes
			    (string-to-number (match-string 1))))
		     t)
		:other))
	(gc-output "B make-net")
	(delete-process
	 (make-network-process
	  :name "server-client-test" :family 'local :server nil :noquery t
	  :service (expand-file-name name server-socket-dir)))
	(gc-output "A make-net")
	t)
    (file-error nil)))


Now do
M-x server-start RET

it won't start a server, but in the it will print in the *scratch*
buffer the GC results before and after the `(delete-process (make-network-process ' call.
There's a leak of 22 conses.

Adding this at the end of process.c:remove_process:

  {
    struct Lisp_Process *p = XPROCESS (proc);
    /* Set all Lisp_Object members of struct Lisp_Process to nil.  */
    p->tty_name = Qnil;
    p->name = Qnil;
    p->command = Qnil;
    p->filter = Qnil;
    p->sentinel = Qnil;
    p->log = Qnil;
    p->buffer = Qnil;
    p->childp = Qnil;
    p->plist = Qnil;
    p->type = Qnil;
    p->mark = Qnil;
    p->status = Qnil;
    p->decode_coding_system = Qnil;
    p->decoding_buf = Qnil;
    p->encode_coding_system = Qnil;
    p->encoding_buf = Qnil;
  }

reduces the leak to 4 conses.  Not sure if doing that is TRTD...

Opinions?

Re: leak in make-network-process

[Dan Nicolaescu]

Dan Nicolaescu <dann@ics.uci.edu> writes:

  > Do 
  > 
  > emacs -Q --daemon
  > (to make sure there's a  server running)
  > 
  > then 
  > 
  > emacs -Q -l test.el 
  > 
  > 
  > cat test.el
  > 
  > (defun gc-output (arg)
  >   (interactive "M")
  >   (with-current-buffer "*scratch*"
  >     (insert (format "%s %S\n" arg
  > 		    (garbage-collect)))))
  > 
  > (defun server-running-p (&optional name)
  >   "Test whether server NAME is running.
  > 
  > Return values:
  >   nil              the server is definitely not running.
  >   t                the server seems to be running.
  >   something else   we cannot determine whether it's running without using
  >                    commands which may have to wait for a long time."
  >   (interactive
  >    (list (if current-prefix-arg
  > 	     (read-string "Server name: " nil nil server-name))))
  >   (unless name (setq name server-name))
  >   (condition-case nil
  >       (if server-use-tcp
  > 	  (with-temp-buffer
  > 	    (insert-file-contents-literally (expand-file-name name server-auth-dir))
  > 	    (or (and (looking-at "127\.0\.0\.1:[0-9]+ \\([0-9]+\\)")
  > 		     (assq 'comm
  > 			   (system-process-attributes
  > 			    (string-to-number (match-string 1))))
  > 		     t)
  > 		:other))
  > 	(gc-output "B make-net")
  > 	(delete-process
  > 	 (make-network-process
  > 	  :name "server-client-test" :family 'local :server nil :noquery t
  > 	  :service (expand-file-name name server-socket-dir)))
  > 	(gc-output "A make-net")
  > 	t)
  >     (file-error nil)))
  > 
  > 
  > Now do
  > M-x server-start RET
  > 
  > it won't start a server, but in the it will print in the *scratch*
  > buffer the GC results before and after the `(delete-process (make-network-process ' call.
  > There's a leak of 22 conses.
  > 
  > Adding this at the end of process.c:remove_process:
  > 
  >   {
  >     struct Lisp_Process *p = XPROCESS (proc);
  >     /* Set all Lisp_Object members of struct Lisp_Process to nil.  */
  >     p->tty_name = Qnil;
  >     p->name = Qnil;
  >     p->command = Qnil;
  >     p->filter = Qnil;
  >     p->sentinel = Qnil;
  >     p->log = Qnil;
  >     p->buffer = Qnil;
  >     p->childp = Qnil;
  >     p->plist = Qnil;
  >     p->type = Qnil;
  >     p->mark = Qnil;
  >     p->status = Qnil;
  >     p->decode_coding_system = Qnil;
  >     p->decoding_buf = Qnil;
  >     p->encode_coding_system = Qnil;
  >     p->encoding_buf = Qnil;
  >   }
  > 
  > reduces the leak to 4 conses.  Not sure if doing that is TRTD...

Actually only this is needed:

  {
    struct Lisp_Process *p = XPROCESS (proc);
    /* Set all Lisp_Object members of struct Lisp_Process to nil.  */
    p->childp = Qnil;
    p->status = Qnil;
  }


p->childp is a plist in the case of network processes
p->status is set to a cons by Fdelete_process for network processes

Why can't GC get these fields?  Should something like this be checked
in?

Re: leak in make-network-process

[Chong Yidong]

Dan Nicolaescu <dann@ics.uci.edu> writes:

> Actually only this is needed:
>
>   {
>     struct Lisp_Process *p = XPROCESS (proc);
>     /* Set all Lisp_Object members of struct Lisp_Process to nil.  */
>     p->childp = Qnil;
>     p->status = Qnil;
>   }
>
> Why can't GC get these fields?  Should something like this be checked
> in?

Maybe something else in Lisp is still referring to that dead process
object.

Re: leak in make-network-process

[Stefan Monnier]

>> Actually only this is needed:
>> 
>> {
>> struct Lisp_Process *p = XPROCESS (proc);
>> /* Set all Lisp_Object members of struct Lisp_Process to nil.  */
p-> childp = Qnil;
p-> status = Qnil;
>> }
>> 
>> Why can't GC get these fields?  Should something like this be checked
>> in?

> Maybe something else in Lisp is still referring to that dead process
> object.

In any case, I think it's OK to explicitly set those fields to nil.
For objects that can be killed (and not revived) like frames, terminals,
processes, it's OK to zero out (some of) their fields to make sure that
even if someone holds on to them, what they referred to can be GC'd (as
long as it makes no other visible difference: e.g. frame parameters
should be nil'd).


        Stefan