Ispell loads dict twice.

Ispell loads dict twice.

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 1239 bytes --]


  Hi !

  I use the latest CVS version.

  With the following :

$ emacs -Q

C-u M-x ispell-change-dictionary RET francais RET
C-x b test1 RET
M-x ispell-change-dictionary RET english RET
M-x ispell-buffer
C-x b test2 RET
M-x ispell-change-dictionary RET english RET

  The « english » dictionary will be loaded twice: on the
  « ispell-buffer » and on the latest « ispell-change-dictionary ».

  The rest of this message is IMHO :-)

  What happens is the following on the second ispell-change-dictionary:

  - ispell-buffer-local-dict is called,
  - this one calls ispell-internal-change-dictionary,
  - ispell-local-dictionary being nil, and current dictionary
    (« english ») not being equal to default one (« francais »), the
    ispell process is killed.

  So it has to be restarted.

  The call to ispell-buffer-local-dict is mandatory
  (ispell-change-dictionary could be called with "" just to load
  default buffer's one), but it should not call
  ispell-internal-change-dictionary at this time.

  The proposed patch :
  - Delays this call,
  - Makes ispell-internal-change-dictionary checks for pdict,
  - Fixes the bug.
  
  Could someone have a look and install it if needed ?


[-- Attachment #1.1.2: Patch for ispell --]
[-- Type: text/x-patch, Size: 5627 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/ChangeLog,v
retrieving revision 1.9435
diff -c -r1.9435 ChangeLog
*** ChangeLog	23 Apr 2006 21:45:28 -0000	1.9435
--- ChangeLog	24 Apr 2006 19:30:55 -0000
***************
*** 1,3 ****
--- 1,14 ----
+ 2006-04-24  Michaël Cadilhac  <michael.cadilhac@lrde.org>
+ 
+ 	* textmodes/ispell.el (ispell-buffer-local-dict): Add a `no-reload'
+ 	argument to avoid the call to `ispell-internal-change-dictionary'
+ 	when not needed.
+ 	(ispell-change-dictionary): Use this argument and call
+ 	`ispell-internal-change-dictionary' after the possible change
+ 	to `ispell-local-dictionary'.
+ 	(ispell-internal-change-dictionary): Check for a change in
+ 	personal dictionary use too.
+ 	
  2006-04-23  Michael Albinus  <michael.albinus@gmx.de>
  
  	* net/tramp.el (tramp-register-file-name-handlers): New defun.
Index: textmodes/ispell.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/textmodes/ispell.el,v
retrieving revision 1.197
diff -c -r1.197 ispell.el
*** textmodes/ispell.el	6 Apr 2006 19:20:37 -0000	1.197
--- textmodes/ispell.el	24 Apr 2006 19:30:55 -0000
***************
*** 2607,2621 ****
  	       (mapcar 'list (ispell-valid-dictionary-list)))
  	  nil t)
  	 current-prefix-arg))
!   (unless arg (ispell-buffer-local-dict))
    (if (equal dict "default") (setq dict nil))
    ;; This relies on completing-read's bug of returning "" for no match
    (cond ((equal dict "")
  	 (message "Using %s dictionary"
  		  (or ispell-local-dictionary ispell-dictionary "default")))
  	((equal dict (or ispell-local-dictionary
  			 ispell-dictionary "default"))
! 	 ;; Specified dictionary is the default already.  No-op
  	 (and (interactive-p)
  	      (message "No change, using %s dictionary" dict)))
  	(t				; reset dictionary!
--- 2607,2624 ----
  	       (mapcar 'list (ispell-valid-dictionary-list)))
  	  nil t)
  	 current-prefix-arg))
!   (unless arg (ispell-buffer-local-dict t))
    (if (equal dict "default") (setq dict nil))
    ;; This relies on completing-read's bug of returning "" for no match
    (cond ((equal dict "")
+ 	 (ispell-internal-change-dictionary)
  	 (message "Using %s dictionary"
  		  (or ispell-local-dictionary ispell-dictionary "default")))
  	((equal dict (or ispell-local-dictionary
  			 ispell-dictionary "default"))
! 	 ;; Specified dictionary is the default already. Could reload
! 	 ;; the dictionaries if needed.
! 	 (ispell-internal-change-dictionary)
  	 (and (interactive-p)
  	      (message "No change, using %s dictionary" dict)))
  	(t				; reset dictionary!
***************
*** 2634,2646 ****
  		  dict))))
  
  (defun ispell-internal-change-dictionary ()
!   "Update the dictionary actually used by Ispell.
  This may kill the Ispell process; if so,
  a new one will be started when needed."
!   (let ((dict (or ispell-local-dictionary ispell-dictionary)))
!     (unless (equal ispell-current-dictionary dict)
        (ispell-kill-ispell t)
!       (setq ispell-current-dictionary dict))))
  
  ;;; Spelling of comments are checked when ispell-check-comments is non-nil.
  
--- 2637,2652 ----
  		  dict))))
  
  (defun ispell-internal-change-dictionary ()
!   "Update the dictionary and the personal dictionary used by Ispell.
  This may kill the Ispell process; if so,
  a new one will be started when needed."
!   (let ((dict (or ispell-local-dictionary ispell-dictionary))
! 	(pdict (or ispell-local-pdict ispell-personal-dictionary)))
!     (unless (and (equal ispell-current-dictionary dict)
! 		 (equal ispell-current-personal-dictionary pdict))
        (ispell-kill-ispell t)
!       (setq ispell-current-dictionary dict
! 	    ispell-current-personal-dictionary pdict))))
  
  ;;; Spelling of comments are checked when ispell-check-comments is non-nil.
  
***************
*** 3667,3674 ****
  
  ;;; Can kill the current ispell process
  
! (defun ispell-buffer-local-dict ()
    "Initializes local dictionary and local personal dictionary.
  When a dictionary is defined in the buffer (see variable
  `ispell-dictionary-keyword'), it will override the local setting
  from \\[ispell-change-dictionary].
--- 3673,3681 ----
  
  ;;; Can kill the current ispell process
  
! (defun ispell-buffer-local-dict (&optional no-reload)
    "Initializes local dictionary and local personal dictionary.
+ If optional NO-RELOAD is non-nil, do not make any dictionary reloading.
  When a dictionary is defined in the buffer (see variable
  `ispell-dictionary-keyword'), it will override the local setting
  from \\[ispell-change-dictionary].
***************
*** 3695,3706 ****
  	    (if (re-search-forward " *\\([^ \"]+\\)" end t)
  		(setq ispell-local-pdict
  		      (match-string-no-properties 1)))))))
!   ;; Reload if new personal dictionary defined.
!   (if (not (equal ispell-current-personal-dictionary
! 		  (or ispell-local-pdict ispell-personal-dictionary)))
!       (ispell-kill-ispell t))
!   ;; Reload if new dictionary defined.
!   (ispell-internal-change-dictionary))
  
  
  (defun ispell-buffer-local-words ()
--- 3702,3710 ----
  	    (if (re-search-forward " *\\([^ \"]+\\)" end t)
  		(setq ispell-local-pdict
  		      (match-string-no-properties 1)))))))
!   (unless no-reload
!     ;; Reload if new dictionary (maybe the personal one) defined.
!     (ispell-internal-change-dictionary)))
  
  
  (defun ispell-buffer-local-words ()

[-- Attachment #1.1.3: Type: text/plain, Size: 336 bytes --]


  Thanks.

-- 
 |      Michaël `Micha' Cadilhac   |   Mieux vaut se taire                  |
 |         Epita/LRDE Promo 2007   |    Que de parler trop fort.            |
 | http://www.lrde.org/~cadilh_m   |            -- As de trèfle             |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Ispell loads dict twice.

[Agustin Martin]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

On Mon, Apr 24, 2006 at 09:36:40PM +0200, Michaël Cadilhac wrote:
> $ emacs -Q
> 
> C-u M-x ispell-change-dictionary RET francais RET
> C-x b test1 RET
> M-x ispell-change-dictionary RET english RET
> M-x ispell-buffer
> C-x b test2 RET
> M-x ispell-change-dictionary RET english RET
> 
>   The « english » dictionary will be loaded twice: on the
>   « ispell-buffer » and on the latest « ispell-change-dictionary ».

While the above problem looks rather minor your patch seems to work
well here, both the above and the personal dict part. I would only
suggest a very minor cosmetic change, as in attached patch (to be
applied on top of your patch), just to help people reading the code
to understand how that call works without looking at 
`ispell-buffer-local-dict' definition.

If this is used along with your patch, no specific changelog entry is
needed, otherwise something like

(ispell-change-dictionary)
 Make (ispell-buffer-local-dict) call more verbose

might suffice

-- 
Agustin

[-- Attachment #2: ispell.el.double_call+pdict+changes.diff --]
[-- Type: text/plain, Size: 459 bytes --]

--- ispell.el.0	2006-04-25 12:20:28.000000000 +0200
+++ ispell.el	2006-04-25 12:29:49.000000000 +0200
@@ -2577,7 +2577,7 @@
 	       (mapcar 'list (ispell-valid-dictionary-list)))
 	  nil t)
 	 current-prefix-arg))
-  (unless arg (ispell-buffer-local-dict t))
+  (unless arg (ispell-buffer-local-dict 'no-reload))
   (if (equal dict "default") (setq dict nil))
   ;; This relies on completing-read's bug of returning "" for no match
   (cond ((equal dict "")

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Ispell loads dict twice.

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 1011 bytes --]

Agustin Martin <agustin.martin@hispalinux.es> writes:

> On Mon, Apr 24, 2006 at 09:36:40PM +0200, Michaël Cadilhac wrote:
>> $ emacs -Q
>> 
>> C-u M-x ispell-change-dictionary RET francais RET
>> C-x b test1 RET
>> M-x ispell-change-dictionary RET english RET
>> M-x ispell-buffer
>> C-x b test2 RET
>> M-x ispell-change-dictionary RET english RET
>> 
>>   The « english » dictionary will be loaded twice: on the
>>   « ispell-buffer » and on the latest « ispell-change-dictionary ».
>
> While the above problem looks rather minor

  In fact, it wasn't for me :-) I frequently switch between french and
  english dictionaries and this bug arises more than you think :-)

> your patch seems to work well here, both the above and the personal
> dict part.
> I would only suggest a very minor cosmetic change

  Thank you, I dithered about making this change in the first patch,
  as I wasn't sure that it was OK regarding to the coding style.

  Here's attached the patch modified.


[-- Attachment #1.1.2: Ispell patch --]
[-- Type: text/x-patch, Size: 5717 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/ChangeLog,v
retrieving revision 1.9435
diff -c -r1.9435 ChangeLog
*** ChangeLog	23 Apr 2006 21:45:28 -0000	1.9435
--- ChangeLog	26 Apr 2006 21:55:49 -0000
***************
*** 1,3 ****
--- 1,16 ----
+ 2006-04-24  Michaël Cadilhac  <michael.cadilhac@lrde.org>
+ 
+ 	* textmodes/ispell.el (ispell-buffer-local-dict): Add a `no-reload'
+ 	argument to avoid the call to `ispell-internal-change-dictionary'
+ 	when not needed.
+ 	(ispell-change-dictionary): Use this argument and call
+ 	`ispell-internal-change-dictionary' after the possible change
+ 	to `ispell-local-dictionary'.
+ 	(ispell-internal-change-dictionary): Check for a change in
+ 	personal dictionary use too.
+ 	Cosmetic changes thanks to Agustin Martin
+ 	<agustin.martin@hispalinux.es>.
+ 
  2006-04-23  Michael Albinus  <michael.albinus@gmx.de>
  
  	* net/tramp.el (tramp-register-file-name-handlers): New defun.
Index: textmodes/ispell.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/textmodes/ispell.el,v
retrieving revision 1.197
diff -c -r1.197 ispell.el
*** textmodes/ispell.el	6 Apr 2006 19:20:37 -0000	1.197
--- textmodes/ispell.el	26 Apr 2006 21:55:50 -0000
***************
*** 2607,2621 ****
  	       (mapcar 'list (ispell-valid-dictionary-list)))
  	  nil t)
  	 current-prefix-arg))
!   (unless arg (ispell-buffer-local-dict))
    (if (equal dict "default") (setq dict nil))
    ;; This relies on completing-read's bug of returning "" for no match
    (cond ((equal dict "")
  	 (message "Using %s dictionary"
  		  (or ispell-local-dictionary ispell-dictionary "default")))
  	((equal dict (or ispell-local-dictionary
  			 ispell-dictionary "default"))
! 	 ;; Specified dictionary is the default already.  No-op
  	 (and (interactive-p)
  	      (message "No change, using %s dictionary" dict)))
  	(t				; reset dictionary!
--- 2607,2624 ----
  	       (mapcar 'list (ispell-valid-dictionary-list)))
  	  nil t)
  	 current-prefix-arg))
!   (unless arg (ispell-buffer-local-dict 'no-reload))
    (if (equal dict "default") (setq dict nil))
    ;; This relies on completing-read's bug of returning "" for no match
    (cond ((equal dict "")
+ 	 (ispell-internal-change-dictionary)
  	 (message "Using %s dictionary"
  		  (or ispell-local-dictionary ispell-dictionary "default")))
  	((equal dict (or ispell-local-dictionary
  			 ispell-dictionary "default"))
! 	 ;; Specified dictionary is the default already. Could reload
! 	 ;; the dictionaries if needed.
! 	 (ispell-internal-change-dictionary)
  	 (and (interactive-p)
  	      (message "No change, using %s dictionary" dict)))
  	(t				; reset dictionary!
***************
*** 2634,2646 ****
  		  dict))))
  
  (defun ispell-internal-change-dictionary ()
!   "Update the dictionary actually used by Ispell.
  This may kill the Ispell process; if so,
  a new one will be started when needed."
!   (let ((dict (or ispell-local-dictionary ispell-dictionary)))
!     (unless (equal ispell-current-dictionary dict)
        (ispell-kill-ispell t)
!       (setq ispell-current-dictionary dict))))
  
  ;;; Spelling of comments are checked when ispell-check-comments is non-nil.
  
--- 2637,2652 ----
  		  dict))))
  
  (defun ispell-internal-change-dictionary ()
!   "Update the dictionary and the personal dictionary used by Ispell.
  This may kill the Ispell process; if so,
  a new one will be started when needed."
!   (let ((dict (or ispell-local-dictionary ispell-dictionary))
! 	(pdict (or ispell-local-pdict ispell-personal-dictionary)))
!     (unless (and (equal ispell-current-dictionary dict)
! 		 (equal ispell-current-personal-dictionary pdict))
        (ispell-kill-ispell t)
!       (setq ispell-current-dictionary dict
! 	    ispell-current-personal-dictionary pdict))))
  
  ;;; Spelling of comments are checked when ispell-check-comments is non-nil.
  
***************
*** 3667,3674 ****
  
  ;;; Can kill the current ispell process
  
! (defun ispell-buffer-local-dict ()
    "Initializes local dictionary and local personal dictionary.
  When a dictionary is defined in the buffer (see variable
  `ispell-dictionary-keyword'), it will override the local setting
  from \\[ispell-change-dictionary].
--- 3673,3681 ----
  
  ;;; Can kill the current ispell process
  
! (defun ispell-buffer-local-dict (&optional no-reload)
    "Initializes local dictionary and local personal dictionary.
+ If optional NO-RELOAD is non-nil, do not make any dictionary reloading.
  When a dictionary is defined in the buffer (see variable
  `ispell-dictionary-keyword'), it will override the local setting
  from \\[ispell-change-dictionary].
***************
*** 3695,3706 ****
  	    (if (re-search-forward " *\\([^ \"]+\\)" end t)
  		(setq ispell-local-pdict
  		      (match-string-no-properties 1)))))))
!   ;; Reload if new personal dictionary defined.
!   (if (not (equal ispell-current-personal-dictionary
! 		  (or ispell-local-pdict ispell-personal-dictionary)))
!       (ispell-kill-ispell t))
!   ;; Reload if new dictionary defined.
!   (ispell-internal-change-dictionary))
  
  
  (defun ispell-buffer-local-words ()
--- 3702,3710 ----
  	    (if (re-search-forward " *\\([^ \"]+\\)" end t)
  		(setq ispell-local-pdict
  		      (match-string-no-properties 1)))))))
!   (unless no-reload
!     ;; Reload if new dictionary (maybe the personal one) defined.
!     (ispell-internal-change-dictionary)))
  
  
  (defun ispell-buffer-local-words ()

[-- Attachment #1.1.3: Type: text/plain, Size: 323 bytes --]


-- 
 |      Michaël `Micha' Cadilhac   |   Pour les 35-40 ans, l'humour         |
 |         Epita/LRDE Promo 2007   |        c'est une plus-value.           |
 | http://www.lrde.org/~cadilh_m   |           -- Guillaume L.              |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Ispell loads dict twice.

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 1997 bytes --]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

>   Hi !
>
>   I use the latest CVS version.
>
>   With the following :
>
> $ emacs -Q
>
> C-u M-x ispell-change-dictionary RET francais RET
> C-x b test1 RET
> M-x ispell-change-dictionary RET english RET
> M-x ispell-buffer
> C-x b test2 RET
> M-x ispell-change-dictionary RET english RET
>
>   The « english » dictionary will be loaded twice: on the
>   « ispell-buffer » and on the latest « ispell-change-dictionary ».
>
>   The rest of this message is IMHO :-)
>
>   What happens is the following on the second ispell-change-dictionary:
>
>   - ispell-buffer-local-dict is called,
>   - this one calls ispell-internal-change-dictionary,
>   - ispell-local-dictionary being nil, and current dictionary
>     (« english ») not being equal to default one (« francais »), the
>     ispell process is killed.
>
>   So it has to be restarted.
>
>   The call to ispell-buffer-local-dict is mandatory
>   (ispell-change-dictionary could be called with "" just to load
>   default buffer's one), but it should not call
>   ispell-internal-change-dictionary at this time.
>
>   The proposed patch :
>   - Delays this call,
>   - Makes ispell-internal-change-dictionary checks for pdict,
>   - Fixes the bug.

  Well,  this  bug  is  still  triggered in  some  cases,  but  before
  I investigate, I was  wondering why ispell-kill-process took so much
  time to execute.

  This function does the following:
  - Send EOF to ispell,
  - Read its output if there is, timeout to 1 sec,
  - Kill the process if it's still not,
  - Wait for it to be really killed (sleeping for 0.25 sec between checks).

  I don't want to be ... rude, I'm really a pacifist actually, but why
  not just delete-process it ?

  I often have some flyspellized buffers in English and some others in
  French, the time I have to wait on every C-x o is kind of
  disturbing...

  I propose the following change:
  

[-- Attachment #1.1.2: ispell.patch --]
[-- Type: text/x-patch, Size: 1960 bytes --]

Index: lisp/ChangeLog
===================================================================
RCS file: /sources/emacs/emacs/lisp/ChangeLog,v
retrieving revision 1.9586
diff -c -B -b -r1.9586 ChangeLog
*** lisp/ChangeLog	23 May 2006 11:23:25 -0000	1.9586
--- lisp/ChangeLog	23 May 2006 18:17:41 -0000
***************
*** 1,3 ****
--- 1,9 ----
+ 2006-05-23  Michaël Cadilhac  <michael.cadilhac@lrde.org>
+ 
+ 	* textmodes/ispell.el (ispell-kill-ispell): If ispell has been
+ 	launched asynchronously, delete its process instead of being
+ 	cool.
+ 
  2006-05-23  Thien-Thi Nguyen  <ttn@gnu.org>
  
  	* emacs-lisp/ewoc.el (ewoc-delete): New function.
Index: lisp/textmodes/ispell.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/textmodes/ispell.el,v
retrieving revision 1.199
diff -c -B -b -r1.199 ispell.el
*** lisp/textmodes/ispell.el	21 May 2006 20:25:43 -0000	1.199
--- lisp/textmodes/ispell.el	23 May 2006 18:17:41 -0000
***************
*** 2572,2586 ****
        (or no-error
  	  (error "There is no ispell process running!"))
      (if ispell-async-processp
! 	(progn
! 	  (process-send-eof ispell-process)
! 	  (if (eq (ispell-process-status) 'run)
! 	      (ispell-accept-output 1))
! 	  (if (eq (ispell-process-status) 'run)
! 	      (kill-process ispell-process))
! 	  (while (not (or (eq (ispell-process-status) 'exit)
! 			  (eq (ispell-process-status) 'signal)))
! 	    (sleep-for 0.25)))
        ;; synchronous processes
        (ispell-send-string "\n")		; make sure side effects occurred.
        (kill-buffer ispell-output-buffer)
--- 2572,2578 ----
        (or no-error
  	  (error "There is no ispell process running!"))
      (if ispell-async-processp
! 	(delete-process ispell-process)
        ;; synchronous processes
        (ispell-send-string "\n")		; make sure side effects occurred.
        (kill-buffer ispell-output-buffer)

[-- Attachment #1.1.3: Type: text/plain, Size: 325 bytes --]



-- 
 |      Michaël `Micha' Cadilhac   |   Un certain Blaise Pascal             |
 |         Epita/LRDE Promo 2007   |     etc... etc...                      |
 | http://www.lrde.org/~cadilh_m   |   -- Prévert (Les paris stupides)      |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Ispell loads dict twice.

[Agustin Martin]

On Tue, May 23, 2006 at 08:26:19PM +0200, Michaël Cadilhac wrote:
>   I don't want to be ... rude, I'm really a pacifist actually, but why
>   not just delete-process it ?
> 
>   I often have some flyspellized buffers in English and some others in
>   French, the time I have to wait on every C-x o is kind of
>   disturbing...
> 
>   I propose the following change:

I had some problems when testing it by switching flyspellized buffers:

 - I activate flyspell mode in both files.
 - Select american as language for one and leave default for the other.

When toggling repeteatedly between both buffers with the mouse, at some point
I get the error

...
Ispell process killed
Starting new Ispell process [default] ...
Ispell process killed
Starting new Ispell process [american] ...
Ispell process killed
Starting new Ispell process [default] ...
Error in post-command-hook: (error ispell exited with signal Killed)
Starting new Ispell process [default] ...
Ispell process killed
Starting new Ispell process [american] ...
Error in post-command-hook: (error ispell exited with signal Killed)


-- 
Agustin

delete-process bug (was: Ispell loads dict twice.)

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 1271 bytes --]

Agustin Martin <agustin.martin@hispalinux.es> writes:

> Ispell process killed
> Starting new Ispell process [american] ...
> Error in post-command-hook: (error ispell exited with signal Killed)

This is  not really  due to my  patch. However,  it shows a  real race
condition in process management of Emacs:

Try the following:

(delete-process (start-process "bug" nil "/tmp/sleep"))
(let ((result (call-process "/tmp/sleep")))
  (message "%S" result))

(/tmp/sleep is just "sleep 1")

You'll have, as a result, the message « Killed » ; but the process
called by call-process is _not actually killed_, it's the previous
(deleted) one that was.

The bug is simple:
- The first process is launched asynchronously, then killed
- A synchronous process is created and executed
- HERE, sigchld_handler is called, because of the first process death
- The PID of the first process is searched in:
  - The list of asynchronous process, in which it is no more,
  - As a fall back, it is considered as a synchronous process.
- Then the synchronous process is said to be killed.

... but it isn't.

After an hour of debugging, I can propose a small change that fixes
this bug and lets no room for any other race condition of that kind,
AFAICT.


[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 1302 bytes --]

Index: src/ChangeLog
===================================================================
RCS file: /sources/emacs/emacs/src/ChangeLog,v
retrieving revision 1.5087
diff -c -B -b -r1.5087 ChangeLog
*** src/ChangeLog	24 May 2006 16:58:47 -0000	1.5087
--- src/ChangeLog	25 May 2006 10:55:02 -0000
***************
*** 1,3 ****
--- 1,8 ----
+ 2006-05-25  Michaël Cadilhac  <michael.cadilhac@lrde.org>
+ 
+ 	* process.c (Fdelete_process): Wait for process termination to
+ 	avoid `sigchld_handler' to consider the process to be synchronous.
+ 
  2006-05-24  Luc Teirlinck  <teirllm@auburn.edu>
  
  	* puresize.h (BASE_PURESIZE): Increase to 1210000.
Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -B -b -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	25 May 2006 10:55:02 -0000
***************
*** 800,805 ****
--- 800,806 ----
    else if (XINT (p->infd) >= 0)
      {
        Fkill_process (process, Qnil);
+       wait_for_termination (p->pid);
        /* Do this now, since remove_process will make sigchld_handler do nothing.  */
        p->status
  	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));

[-- Attachment #1.1.3: Type: text/plain, Size: 335 bytes --]


Regards,

-- 
 |      Michaël `Micha' Cadilhac   |   Mieux vaut se taire                  |
 |         Epita/LRDE Promo 2007   |    Que de parler trop fort.            |
 | http://www.lrde.org/~cadilh_m   |            -- As de trèfle             |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug (was: Ispell loads dict twice.)

[Agustin Martin]

On Thu, May 25, 2006 at 12:57:18PM +0200, Michaël Cadilhac wrote:[B
> Agustin Martin <agustin.martin@hispalinux.es> writes:
> 
> > Ispell process killed
> > Starting new Ispell process [american] ...
> > Error in post-command-hook: (error ispell exited with signal Killed)
> 
> This is  not really  due to my  patch. However,  it shows a  real race
> condition in process management of Emacs:

Thanks for clarifying,

I was also guessing so, your patch was working pretty well at home
(Debian stable emacs21.4), but failed randomly at work in both Debian
stable emacs21.4 (tested this morning) and in Debian unstable emacs21.4
and emacs-snapshot, in all cases even in a clean environment.

The only difference left is that my home computer is slower than the one I
use at work.

Besides the race condition, your ispell.el original patch seems very
reasonable to me.

It is a pity that upstream ispell.el mailing list is not archived anywhere,
and that we cannot browse its CVS logs, to check if original code was
intended as a workaround for this kind of race conditions.

> After an hour of debugging, I can propose a small change that fixes
> this bug and lets no room for any other race condition of that kind,
> AFAICT.

I agree that is the race condition what needs to be fixed instead of using
strange workarounds.

Thanks for your feedback,

-- 
Agustin

Re: delete-process bug

[Stefan Monnier]

> This is  not really  due to my  patch. However,  it shows a  real race
> condition in process management of Emacs:
[...]
> After an hour of debugging, I can propose a small change that fixes
> this bug and lets no room for any other race condition of that kind,
> AFAICT.
[...]
> + 	* process.c (Fdelete_process): Wait for process termination to
> + 	avoid `sigchld_handler' to consider the process to be synchronous.

I don't think it's a good idea.  The process might not die in response to
delete-process, so we would end up waiting "indefinitely".

I think in order to avoid such race-conditions without waiting for the
process's death, we'll need to remember those processes that were deleted
but aren't dead yet.  Of course, it goes against the docstring of
`delete-process' which says "forget about it immediately".  But we don't
need to keep the whole process around; just the PID should be enough, kept
in a list of "deleted but not dead" PIDs.  See sample patch below.


        Stefan


--- process.c	12 mai 2006 11:54:58 -0400	1.481
+++ process.c	25 mai 2006 10:51:51 -0400	
@@ -778,6 +778,13 @@
   return proc;
 }
 
+/* Fdelete_process promises to immediately forget about the process, but in
+   reality Emacs needs to remember those processes until they die, otherwise
+   it doesn't know what to do with the SIGCHLD signal and might be tempted
+   to interpret that signal as applying to some other non-deleted
+   process ;-(.  */
+static Lisp_Object live_deleted_processes;
+
 DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
        doc: /* Delete PROCESS: kill it and forget about it immediately.
 PROCESS may be a process, a buffer, the name of a process or buffer, or
@@ -800,6 +807,9 @@
   else if (XINT (p->infd) >= 0)
     {
       Fkill_process (process, Qnil);
+      live_deleted_processes = Fcons (make_number (p->pid),
+				      /* GC previous elements.  */
+				      Fdelq (Qnil, live_deleted_processes));
       /* Do this now, since remove_process will make sigchld_handler do nothing.  */
       p->status
 	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
@@ -6373,6 +6383,13 @@
 
       /* Find the process that signaled us, and record its status.  */
 
+      /* Maybe the process was already passed to Fdelete_process.  */
+      tail = Fmemq (make_number (pid), live_deleted_processes);
+      if (!NILP (tail)) {
+	Fsetcar (tail, Qnil);
+	return;
+      }
+
       p = 0;
       for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
 	{
@@ -7357,6 +7374,7 @@
 void
 init_process ()
 {
+  live_deleted_processes = Qnil;
 }
 
 void

Re: delete-process bug

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I think in order to avoid such race-conditions without waiting for
> the process's death, we'll need to remember those processes that
> were deleted but aren't dead yet.  Of course, it goes against the
> docstring of `delete-process' which says "forget about it
> immediately".  But we don't need to keep the whole process around;
> just the PID should be enough, kept in a list of "deleted but not
> dead" PIDs.

> +static Lisp_Object live_deleted_processes;
> +

"live-deleted"?  How about calling them "undead processes"?  They are
pretty much zombies.

Sounds geekier.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 1813 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This is  not really  due to my  patch. However,  it shows a  real race
>> condition in process management of Emacs:
> [...]
>> After an hour of debugging, I can propose a small change that fixes
>> this bug and lets no room for any other race condition of that kind,
>> AFAICT.
> [...]
>> + 	* process.c (Fdelete_process): Wait for process termination to
>> + 	avoid `sigchld_handler' to consider the process to be synchronous.
>
> I don't think it's a good idea.  The process might not die in response to
> delete-process, so we would end up waiting "indefinitely".

Well, I copied the behavior of call_process, and tried to minimize the
patch.  However, I'm  interested  in knowing  when  a process  doesn't
answer a SIGKILL? I thought it was a non-ignorable signal.

> I think in order to avoid such race-conditions without waiting for the
> process's death, we'll need to remember those processes that were deleted
> but aren't dead yet.  Of course, it goes against the docstring of
> `delete-process' which says "forget about it immediately".  But we don't
> need to keep the whole process around; just the PID should be enough, kept
> in a list of "deleted but not dead" PIDs.  See sample patch below.
>

Actually,  I did  try such  a solution,  but I  wasn't sure  where the
elements of that list should be  deleted (removed? ;-)) ; I'm not very
comfortable with making such things in a sighandler.

-- 
 |      Michaël `Micha' Cadilhac   |     Le copillage-collage               |
 |         Epita/LRDE Promo 2007   |        tue le programmeur.             |
 | http://www.lrde.org/~cadilh_m   |            -- Dictons LRDE             |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[David Kastrup]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> This is  not really  due to my  patch. However,  it shows a  real race
>>> condition in process management of Emacs:
>> [...]
>>> After an hour of debugging, I can propose a small change that fixes
>>> this bug and lets no room for any other race condition of that kind,
>>> AFAICT.
>> [...]
>>> + 	* process.c (Fdelete_process): Wait for process termination to
>>> + 	avoid `sigchld_handler' to consider the process to be synchronous.
>>
>> I don't think it's a good idea.  The process might not die in response to
>> delete-process, so we would end up waiting "indefinitely".
>
> Well, I copied the behavior of call_process, and tried to minimize
> the patch.  However, I'm interested in knowing when a process
> doesn't answer a SIGKILL? I thought it was a non-ignorable signal.

That merely means that the process will never run again.  It puts it
into "zombie" state.  It will not get removed until the scheduler gets
around to start the cleanup, and then the cleanup still has to finish
(all files and sockets have to be closed).  When devices are stuck,
this can take arbitrarily long.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: delete-process bug

[Stefan Monnier]

>> I think in order to avoid such race-conditions without waiting for the
>> process's death, we'll need to remember those processes that were deleted
>> but aren't dead yet.  Of course, it goes against the docstring of
>> `delete-process' which says "forget about it immediately".  But we don't
>> need to keep the whole process around; just the PID should be enough, kept
>> in a list of "deleted but not dead" PIDs.  See sample patch below.

> Actually,  I did  try such  a solution,  but I  wasn't sure  where the
> elements of that list should be  deleted (removed? ;-)) ; I'm not very
> comfortable with making such things in a sighandler.

Check my patch: in the sighandler I only zero-out the list-entry; the
cons-cell itself will only be removed next time you call delete-process.


        Stefan

Re: delete-process bug

[Kim F. Storm]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> +static Lisp_Object live_deleted_processes;

What about deleted_pid_list ?

> +
>  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
>         doc: /* Delete PROCESS: kill it and forget about it immediately.
>  PROCESS may be a process, a buffer, the name of a process or buffer, or
> @@ -800,6 +807,9 @@
>    else if (XINT (p->infd) >= 0)
>      {
>        Fkill_process (process, Qnil);
> +      live_deleted_processes = Fcons (make_number (p->pid),

IIRC, using an EMACS_INT for storing pids was unsafe.  Using a float is better.

> +      tail = Fmemq (make_number (pid), live_deleted_processes);

Ditto.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Richard Stallman]

    > +static Lisp_Object live_deleted_processes;

    What about deleted_pid_list ?

That is a good name.

Re: delete-process bug

[Stefan Monnier]

> IIRC, using an EMACS_INT for storing pids was unsafe.  Using a float
> is better.

Duh, right.  The patch should use make_number_or_float (and Fmember, then).
Thanks,


        Stefan

Re: delete-process bug

[Richard Stallman]

Thanks for working on this.  I agree that `live_deleted_processes'
is a good name.  ("Zombie process" has a different meaning.)

However, maybe there is still a race condition.  Suppose the signal
comes in the middle of the line

    +      live_deleted_processes = Fcons (make_number (p->pid),
    +				      /* GC previous elements.  */
    +				      Fdelq (Qnil, live_deleted_processes));

Suppose it comes between there and the call to remove_process?

Does the right thing happen in all these cases?
If not, maybe it is necessary to block signals starting from
just before setting live_deleted_processes thru after calling
remove_process.


Also, please remember that in Emacs braces go on lines by themselves:

+      tail = Fmemq (make_number (pid), live_deleted_processes);
+      if (!NILP (tail)) {
+	Fsetcar (tail, Qnil);
+	return;
+      }

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 1082 bytes --]

Richard Stallman <rms@gnu.org> writes:

> Thanks for working on this.  I agree that `live_deleted_processes'
> is a good name.  ("Zombie process" has a different meaning.)
>
> However, maybe there is still a race condition.  Suppose the signal
> comes in the middle of the line
>
>     +      live_deleted_processes = Fcons (make_number (p->pid),
>     +				      /* GC previous elements.  */
>     +				      Fdelq (Qnil, live_deleted_processes));
>
> Suppose it comes between there and the call to remove_process?
> Does the right thing happen in all these cases?

I think so. In fact, synchronous  process will be said to be dead, but
as we  are calling at this  moment delete-process, we  are not working
with any synchronous processes.

-- 
 |      Michaël `Micha' Cadilhac   |   All your base are belong to us.      |
 |         Epita/LRDE Promo 2007   |     You have no change to survive      |
 | http://www.lrde.org/~cadilh_m   |        make your time, hahaha.         |
 `--  -   JID: micha@amessage.be --'        -- Zero Wings              -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Richard Stallman]

    > Suppose it comes between there and the call to remove_process?
    > Does the right thing happen in all these cases?

    I think so. In fact, synchronous  process will be said to be dead, but
    as we  are calling at this  moment delete-process, we  are not working
    with any synchronous processes.

Thanks for checking.

Please add a comment explaining why there is no race condition if
SIGCHLD comes in during those lines.

Re: delete-process bug

[Stefan Monnier]

> Thanks for working on this.  I agree that `live_deleted_processes'
> is a good name.  ("Zombie process" has a different meaning.)

> However, maybe there is still a race condition.  Suppose the signal
> comes in the middle of the line

>     +      live_deleted_processes = Fcons (make_number (p->pid),
>     +				      /* GC previous elements.  */
>     +				      Fdelq (Qnil, live_deleted_processes));

> Suppose it comes between there and the call to remove_process?

I think it's OK: if the signal handler comes before the assignment to
live_deleted_processes, then it'll all behave as if the signal came even
before the call to delete-process.  If it comes afterwards, the signal will
be ignored (as if it came after the call to delete-process).

> Does the right thing happen in all these cases?

I believe so.

> Also, please remember that in Emacs braces go on lines by themselves:

> +      tail = Fmemq (make_number (pid), live_deleted_processes);
> +      if (!NILP (tail)) {
> +	Fsetcar (tail, Qnil);
> +	return;
> +      }

I do remember, but indeed I missed this one.  Thanks for spotting it.


        Stefan

Re: delete-process bug

[Michael Mauger]

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> 
> > Thanks for working on this.  I agree that `live_deleted_processes'
> > is a good name.  ("Zombie process" has a different meaning.)
> 
> > However, maybe there is still a race condition.  Suppose the signal
> > comes in the middle of the line
> 
> >     +      live_deleted_processes = Fcons (make_number (p->pid),
> >     +				      /* GC previous elements.  */
> >     +				      Fdelq (Qnil, 
live_deleted_processes));
> 
> > Suppose it comes between there and the call to remove_process?
> 
> I think it's OK: if the signal handler comes before the assignment to
> live_deleted_processes, then it'll all behave as if the signal came even
> before the call to delete-process.  If it comes afterwards, the signal will
> be ignored (as if it came after the call to delete-process).
> 
> > Does the right thing happen in all these cases?
> 
> I believe so.
> 

My instinct would be to add the pid to the list prior to causing the event that 
the list is designed to detect.  Treating the signal as though it happened 
before the call to delete-process (if we don't add the pid to the list first) 
when we know it was most likely caused by delete-process seems to be asking for 
trouble.

Just sayin'...

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 2462 bytes --]

Michael Mauger <mmaug@yahoo.com> writes:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>> 
>> > Thanks for working on this.  I agree that `live_deleted_processes'
>> > is a good name.  ("Zombie process" has a different meaning.)
>> 
>> > However, maybe there is still a race condition.  Suppose the signal
>> > comes in the middle of the line
>> 
>> >     +      live_deleted_processes = Fcons (make_number (p->pid),
>> >     +				      /* GC previous elements.  */
>> >     +				      Fdelq (Qnil, 
> live_deleted_processes));
>> 
>> > Suppose it comes between there and the call to remove_process?
>> 
>> I think it's OK: if the signal handler comes before the assignment to
>> live_deleted_processes, then it'll all behave as if the signal came even
>> before the call to delete-process.  If it comes afterwards, the signal will
>> be ignored (as if it came after the call to delete-process).
>> 
>> > Does the right thing happen in all these cases?
>> 
>> I believe so.
>> 
>
> My instinct would be to add the pid to the list prior to causing the event that 
> the list is designed to detect.  Treating the signal as though it happened 
> before the call to delete-process (if we don't add the pid to the list first) 
> when we know it was most likely caused by delete-process seems to be asking for 
> trouble.

Sounds better, indeed.

However, no race condition could have happened because of a (more)
simple thing (than what I said):

The race condition appears when
1) Emacs got a SIGCHLD
2) and the process has been suppressed from process_list.

But the deletion from process_list is made _after_ the adding to
live_deleted_processes. So no problem here, I think, the process is
always known (so not supposed to be a synchronous one).

If something has to be put in comment to say that no race condition
is triggered, I think something like the following is OK:

``A race condition occurred when the deleted process was no longer in
process_list but still had a SIGCHLD treatment to do. Now, the deleted
process is never thought to be asynchronous.''

Regards

-- 
 |      Michaël `Micha' Cadilhac   |     Le copillage-collage               |
 |         Epita/LRDE Promo 2007   |        tue le programmeur.             |
 | http://www.lrde.org/~cadilh_m   |            -- Dictons LRDE             |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Stefan Monnier]

BTW, did anyone actually test my code to make sure that it does indeed work
and fix the problem?


        Stefan

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> BTW, did anyone actually test my code to make sure that it does indeed work
> and fix the problem?
>
>         Stefan

As far as I tested, it doesn't work and freeze my Emacs. I'll
investigate today.

-- 
 |      Michaël `Micha' Cadilhac   |   Mieux vaut se taire                  |
 |         Epita/LRDE Promo 2007   |    Que de parler trop fort.            |
 | http://www.lrde.org/~cadilh_m   |            -- As de trèfle             |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 2682 bytes --]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> BTW, did anyone actually test my code to make sure that it does indeed work
>> and fix the problem?
>>
>>         Stefan
>
> As far as I tested, it doesn't work and freeze my Emacs. I'll
> investigate today.

I fixed what we discussed here.

I found two bugs in your proposal:
- the list was set to Qnil at the wrong place (wrong init)
- `return' was used in the handler, but it could still have some
   process to treat (a continue would have been better but still
   wrong).

A patch is attached.

However, the whole thing still doesn't work ; Emacs freezes or prints
this kind of error message: 

Error in post-command-hook: (wrong-type-argument listp (nil DEAD . 18764377))

I thought that the race condition could be the following:
- delq makes some change on  the list but not all,
- It is interrupted by the handler,
- The handler now has to deal with a malformed list.

It's certainly a bug too, as with the delq I had some Abort in the
handler of the form:

#0  0xb7a45951 in kill () from /lib/libc.so.6
#1  0x080eb75c in abort () at emacs.c:464
#2  0x081536b2 in Fsignal (error_symbol=137610425, data=137557813) at eval.c:1616
#3  0x081402ce in wrong_type_argument (predicate=137622913, value=149282605) at data.c:124
#4  0x0815c175 in Fmember (elt=71112, list=149282605) at fns.c:1468
#5  0x0818577c in sigchld_handler (signo=17) at process.c:6390

Which I think I don't have without, however, it is not the only error,
as with this line deleted, I still have the previous error and
problems. For example, I don't think it's normal for emacs to receive
a SIGPIPE

#0  0xb7ba62b8 in write () from /lib/libpthread.so.0
#1  0x00000002 in ?? ()
#2  0x0810561e in emacs_write (fildes=12, buf=0x8d26178 "!\n", nbyte=12) at sysdep.c:3370
#3  0x08187afb in send_process (proc=150054228, buf=0x8d26178 "!\n", len=2, object=149437363) at process.c:5489
#4  0x08188274 in Fprocess_send_string (process=150054228, string=149437363) at process.c:5643
#5  0x081534cb in Ffuncall (nargs=3, args=0xbfea69b4) at eval.c:2905

... Alas, most of the backtraces only show Emacs blocked in select, 
pthread things or wait_for_termination.

If any hacker can help :-)


Maybe it's  not the good solution  to keep traces of  these. The other
possibility (if we  don't consider the wait_for_termination solution),
is to store the PID of the synchronous process ; but I'm confused with
compatibilities with DOS and Mac :-/

If we have the PID of the synchronous process, we can explicitly test
it in sigchld_handler.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 6576 bytes --]

Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	28 May 2006 15:29:27 -0000
***************
*** 778,783 ****
--- 778,792 ----
    return proc;
  }
  
+ 
+ /* Fdelete_process promises to immediately forget about the process, but in
+    reality, Emacs needs to remember those processes until they have been
+    treated by sigchld_handler; otherwise this handler would consider the
+    process as being synchronous and say that the synchronous process is
+    dead.  */
+ static Lisp_Object deleted_pid_list;
+ 
+ 
  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
         doc: /* Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
***************
*** 800,805 ****
--- 809,818 ----
    else if (XINT (p->infd) >= 0)
      {
        Fkill_process (process, Qnil);
+       /* No problem storing the pid here, as it is still in Vprocess_alist.  */
+       deleted_pid_list = Fcons (make_fixnum_or_float (p->pid),
+ 				/* GC handled elements set to nil.  */
+ 				Fdelq (Qnil, deleted_pid_list));
        /* Do this now, since remove_process will make sigchld_handler do nothing.  */
        p->status
  	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
***************
*** 6373,6445 ****
  
        /* Find the process that signaled us, and record its status.  */
  
!       p = 0;
!       for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
! 	{
! 	  proc = XCDR (XCAR (tail));
! 	  p = XPROCESS (proc);
! 	  if (GC_EQ (p->childp, Qt) && p->pid == pid)
! 	    break;
  	  p = 0;
! 	}
  
!       /* Look for an asynchronous process whose pid hasn't been filled
! 	 in yet.  */
!       if (p == 0)
! 	for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
! 	  {
! 	    proc = XCDR (XCAR (tail));
! 	    p = XPROCESS (proc);
! 	    if (p->pid == -1)
! 	      break;
! 	    p = 0;
! 	  }
  
!       /* Change the status of the process that was found.  */
!       if (p != 0)
! 	{
! 	  union { int i; WAITTYPE wt; } u;
! 	  int clear_desc_flag = 0;
  
! 	  XSETINT (p->tick, ++process_tick);
! 	  u.wt = w;
! 	  p->raw_status = u.i;
! 	  p->raw_status_new = 1;
! 
! 	  /* If process has terminated, stop waiting for its output.  */
! 	  if ((WIFSIGNALED (w) || WIFEXITED (w))
! 	      && XINT (p->infd) >= 0)
! 	    clear_desc_flag = 1;
  
! 	  /* We use clear_desc_flag to avoid a compiler bug in Microsoft C.  */
! 	  if (clear_desc_flag)
! 	    {
! 	      FD_CLR (XINT (p->infd), &input_wait_mask);
! 	      FD_CLR (XINT (p->infd), &non_keyboard_wait_mask);
! 	    }
  
! 	  /* Tell wait_reading_process_output that it needs to wake up and
! 	     look around.  */
! 	  if (input_available_clear_time)
! 	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
! 	}
  
! 	/* There was no asynchronous process found for that id.  Check
! 	   if we have a synchronous process.  */
!       else
! 	{
! 	  synch_process_alive = 0;
  
! 	  /* Report the status of the synchronous process.  */
! 	  if (WIFEXITED (w))
! 	    synch_process_retcode = WRETCODE (w);
! 	  else if (WIFSIGNALED (w))
!             synch_process_termsig = WTERMSIG (w);
! 
! 	  /* Tell wait_reading_process_output that it needs to wake up and
! 	     look around.  */
! 	  if (input_available_clear_time)
! 	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
        /* On some systems, we must return right away.
--- 6386,6466 ----
  
        /* Find the process that signaled us, and record its status.  */
  
!       /* The process can have been deleted by Fdelete_process.  */
!       tail = Fmember (make_fixnum_or_float (pid), deleted_pid_list);
!       if (!NILP (tail))
! 	Fsetcar (tail, Qnil);
!       else
!         {
! 	  /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
  	  p = 0;
! 	  for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
! 	    {
! 	      proc = XCDR (XCAR (tail));
! 	      p = XPROCESS (proc);
! 	      if (GC_EQ (p->childp, Qt) && p->pid == pid)
! 		break;
! 	      p = 0;
! 	    }
  
! 	  /* Look for an asynchronous process whose pid hasn't been filled
! 	     in yet.  */
! 	  if (p == 0)
! 	    for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
! 	      {
! 		proc = XCDR (XCAR (tail));
! 		p = XPROCESS (proc);
! 		if (p->pid == -1)
! 		  break;
! 		p = 0;
! 	      }
  
! 	  /* Change the status of the process that was found.  */
! 	  if (p != 0)
! 	    {
! 	      union { int i; WAITTYPE wt; } u;
! 	      int clear_desc_flag = 0;
  
! 	      XSETINT (p->tick, ++process_tick);
! 	      u.wt = w;
! 	      p->raw_status = u.i;
! 	      p->raw_status_new = 1;
! 
! 	      /* If process has terminated, stop waiting for its output.  */
! 	      if ((WIFSIGNALED (w) || WIFEXITED (w))
! 		  && XINT (p->infd) >= 0)
! 		clear_desc_flag = 1;
  
! 	      /* We use clear_desc_flag to avoid a compiler bug in Microsoft C.  */
! 	      if (clear_desc_flag)
! 		{
! 		  FD_CLR (XINT (p->infd), &input_wait_mask);
! 		  FD_CLR (XINT (p->infd), &non_keyboard_wait_mask);
! 		}
  
! 	      /* Tell wait_reading_process_output that it needs to wake up and
! 		 look around.  */
! 	      if (input_available_clear_time)
! 		EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
! 	    }
  
! 	  /* There was no asynchronous process found for that id.  Check
! 	     if we have a synchronous process.  */
! 	  else
! 	    {
! 	      synch_process_alive = 0;
  
! 	      /* Report the status of the synchronous process.  */
! 	      if (WIFEXITED (w))
! 		synch_process_retcode = WRETCODE (w);
! 	      else if (WIFSIGNALED (w))
! 		synch_process_termsig = WTERMSIG (w);
! 
! 	      /* Tell wait_reading_process_output that it needs to wake up and
! 		 look around.  */
! 	      if (input_available_clear_time)
! 		EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
! 	    }
  	}
  
        /* On some systems, we must return right away.
***************
*** 6843,6848 ****
--- 6864,6870 ----
    FD_SET (0, &input_wait_mask);
  
    Vprocess_alist = Qnil;
+   deleted_pid_list = Qnil;
    for (i = 0; i < MAXDESC; i++)
      {
        chan_process[i] = Qnil;

[-- Attachment #1.1.3: Type: text/plain, Size: 325 bytes --]

  
-- 
 |      Michaël `Micha' Cadilhac   |   Mieux vaut se taire                  |
 |         Epita/LRDE Promo 2007   |    Que de parler trop fort.            |
 | http://www.lrde.org/~cadilh_m   |            -- As de trèfle             |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Stefan Monnier]

> Error in post-command-hook: (wrong-type-argument listp (nil DEAD . 18764377))

Oh yes, the live_deleted_processes variable needs to be gcpro'd.


        Stefan

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 697 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Error in post-command-hook: (wrong-type-argument listp (nil DEAD . 18764377))
>
> Oh yes, the live_deleted_processes variable needs to be gcpro'd.

I'm not  familiar with Emacs C code,  so I don't know  how to proceed.
Can you send a patch, or at least, the lines that have to be modified,
and how ?

Thanks.

-- 
 |      Michaël `Micha' Cadilhac   |   Pour les 35-40 ans, l'humour         |
 |         Epita/LRDE Promo 2007   |        c'est une plus-value.           |
 | http://www.lrde.org/~cadilh_m   |           -- Guillaume L.              |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Stefan Monnier]

>>> Error in post-command-hook: (wrong-type-argument listp (nil DEAD . 18764377))
>> 
>> Oh yes, the live_deleted_processes variable needs to be gcpro'd.

> I'm not  familiar with Emacs C code,  so I don't know  how to proceed.
> Can you send a patch, or at least, the lines that have to be modified,
> and how ?

Add something like

   staticpro (&live_deleted_processes);

in syms_of_process


        Stefan

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 1156 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> Error in post-command-hook: (wrong-type-argument listp (nil DEAD . 18764377))
>>> 
>>> Oh yes, the live_deleted_processes variable needs to be gcpro'd.
>
>> I'm not  familiar with Emacs C code,  so I don't know  how to proceed.
>> Can you send a patch, or at least, the lines that have to be modified,
>> and how ?
>
> Add something like
>
>    staticpro (&live_deleted_processes);
>
> in syms_of_process

Works fine!

I used two tests:

  1) (original test) Quick switching between two flyspellized buffers
     that have different dictionaries,

  2) 

(let (processes)
  (dotimes (i 100)
    (push (start-process "bug" nil "/tmp/sleep") processes))
  (dotimes (i 50)
    (if (zerop (mod i 2))
	(delete-process (pop processes))
      (kill-process (pop processes))))
  (message "%S" (call-process "/tmp/sleep"))
  (dotimes (i 50)
    (delete-process (pop processes)))
  (message "%S" (call-process "/tmp/sleep")))

None of them succeeded before, but now it's OK.

(Final ?) patch attached.

Agustin, can you check that you can't trigger the bug anymore?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 3458 bytes --]

Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	28 May 2006 20:24:11 -0000
***************
*** 778,783 ****
--- 778,792 ----
    return proc;
  }
  
+ 
+ /* Fdelete_process promises to immediately forget about the process, but in
+    reality, Emacs needs to remember those processes until they have been
+    treated by sigchld_handler; otherwise this handler would consider the
+    process as being synchronous and say that the synchronous process is
+    dead.  */
+ static Lisp_Object deleted_pid_list;
+ 
+ 
  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
         doc: /* Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
***************
*** 800,805 ****
--- 809,818 ----
    else if (XINT (p->infd) >= 0)
      {
        Fkill_process (process, Qnil);
+       /* No problem storing the pid here, as it is still in Vprocess_alist.  */
+       deleted_pid_list = Fcons (make_fixnum_or_float (p->pid),
+ 				/* GC treated elements set to nil.  */
+ 				Fdelq (Qnil, deleted_pid_list));
        /* Do this now, since remove_process will make sigchld_handler do nothing.  */
        p->status
  	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
***************
*** 6373,6378 ****
--- 6386,6400 ----
  
        /* Find the process that signaled us, and record its status.  */
  
+       /* The process can have been deleted by Fdelete_process.  */
+       tail = Fmember (make_fixnum_or_float (pid), deleted_pid_list);
+       if (!NILP (tail))
+ 	{
+ 	  Fsetcar (tail, Qnil);
+ 	  goto sigchld_end_of_loop;
+ 	}
+ 
+       /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
        p = 0;
        for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
  	{
***************
*** 6424,6431 ****
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
! 	/* There was no asynchronous process found for that id.  Check
! 	   if we have a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
--- 6446,6453 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
!       /* There was no asynchronous process found for that pid: we have
! 	 a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
***************
*** 6442,6447 ****
--- 6464,6473 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
+ 
+     sigchld_end_of_loop:
+       ;
+ 
        /* On some systems, we must return right away.
  	 If any more processes want to signal us, we will
  	 get another signal.
***************
*** 6843,6848 ****
--- 6869,6875 ----
    FD_SET (0, &input_wait_mask);
  
    Vprocess_alist = Qnil;
+   deleted_pid_list = Qnil;
    for (i = 0; i < MAXDESC; i++)
      {
        chan_process[i] = Qnil;
***************
*** 6981,6986 ****
--- 7008,7014 ----
    staticpro (&Qlast_nonmenu_event);
  
    staticpro (&Vprocess_alist);
+   staticpro (&deleted_pid_list);
  
    DEFVAR_BOOL ("delete-exited-processes", &delete_exited_processes,
  	       doc: /* *Non-nil means delete processes immediately when they exit.

[-- Attachment #1.1.3: Type: text/plain, Size: 323 bytes --]


-- 
 |      Michaël `Micha' Cadilhac   |   Would someone please DTRT with this  |
 |         Epita/LRDE Promo 2007   |         then ACK?                      |
 | http://www.lrde.org/~cadilh_m   |           -- Richard Stallman          |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> (Final ?) patch attached.

Looks good to me!


I wonder whether the pid should be added to deleted_pid_list if SIGCHLD is
not defined?  In that case, it would be a memory leak...

But I also wonder if any of the async process stuff actually works
without SIGCHLD ?!?   Are there any systems which support async processes
but don't have SIGCHLD?


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 1498 bytes --]

storm@cua.dk (Kim F. Storm) writes:

> michael.cadilhac@lrde.org (Michaël Cadilhac) writes:
>
>> (Final ?) patch attached.
>
> Looks good to me!
>
> I wonder whether the pid should be added to deleted_pid_list if SIGCHLD is
> not defined?  In that case, it would be a memory leak...

Oh, hmm... About memory leaks hrm...

In fact, there is already a configuration that would lead to the non
deletion of the PID from the list:

- Call to delete_process,
- Interrupted by a signal before inserting  in deleted_processes_list ,
- Deletion from  process_alist,
- Then delete_process continues by... inserting the dead PID in the
    deleted_processes_list.

I forgot to tell about it in my previous post. Sorry.

Any idea for fixing this ? Putting the insertion in
deleted_processes_list BEFORE the Kill would limit this leak, but not
completely avoid it.


> But I also wonder if any of the async process stuff actually works
> without SIGCHLD ?!?   Are there any systems which support async processes
> but don't have SIGCHLD?

Indeed, I  don't know any system  in which SIGCHLD is  not defined and
that  can  use sub  processes  (are  we  talking about  a  one-process
system ?!).

But it would be cleaner, indeed, to define the whole thing iff SIGCHLD
is defined.

I modified the patch for that (including in sigchld_handler and so
because the list is not defined if SIGCHLD is not), and to make the
insertion in deleted_processes_list before the kill.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 3541 bytes --]

Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	28 May 2006 21:35:32 -0000
***************
*** 778,783 ****
--- 778,793 ----
    return proc;
  }
  
+ 
+ #ifdef SIGCHLD
+ /* Fdelete_process promises to immediately forget about the process, but in
+    reality, Emacs needs to remember those processes until they have been
+    treated by sigchld_handler; otherwise this handler would consider the
+    process as being synchronous and say that the synchronous process is
+    dead.  */
+ static Lisp_Object deleted_pid_list;
+ #endif
+ 
  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
         doc: /* Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
***************
*** 799,804 ****
--- 809,820 ----
      }
    else if (XINT (p->infd) >= 0)
      {
+ #ifdef SIGCHLD
+       /* No problem storing the pid here, as it is still in Vprocess_alist.  */
+       deleted_pid_list = Fcons (make_fixnum_or_float (p->pid),
+ 				/* GC treated elements set to nil.  */
+ 				Fdelq (Qnil, deleted_pid_list));
+ #end
        Fkill_process (process, Qnil);
        /* Do this now, since remove_process will make sigchld_handler do nothing.  */
        p->status
***************
*** 6373,6378 ****
--- 6389,6405 ----
  
        /* Find the process that signaled us, and record its status.  */
  
+ #ifdef SIGCHLD
+       /* The process can have been deleted by Fdelete_process.  */
+       tail = Fmember (make_fixnum_or_float (pid), deleted_pid_list);
+       if (!NILP (tail))
+ 	{
+ 	  Fsetcar (tail, Qnil);
+ 	  goto sigchld_end_of_loop;
+ 	}
+ #endif
+ 
+       /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
        p = 0;
        for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
  	{
***************
*** 6424,6431 ****
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
! 	/* There was no asynchronous process found for that id.  Check
! 	   if we have a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
--- 6451,6458 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
!       /* There was no asynchronous process found for that pid: we have
! 	 a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
***************
*** 6442,6447 ****
--- 6469,6478 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
+ 
+     sigchld_end_of_loop:
+       ;
+ 
        /* On some systems, we must return right away.
  	 If any more processes want to signal us, we will
  	 get another signal.
***************
*** 6843,6848 ****
--- 6874,6882 ----
    FD_SET (0, &input_wait_mask);
  
    Vprocess_alist = Qnil;
+ #ifdef SIGCHLD
+   deleted_pid_list = Qnil;
+ #endif
    for (i = 0; i < MAXDESC; i++)
      {
        chan_process[i] = Qnil;
***************
*** 6981,6986 ****
--- 7015,7023 ----
    staticpro (&Qlast_nonmenu_event);
  
    staticpro (&Vprocess_alist);
+ #ifdef SIGCHLD
+   staticpro (&deleted_pid_list);
+ #endif
  
    DEFVAR_BOOL ("delete-exited-processes", &delete_exited_processes,
  	       doc: /* *Non-nil means delete processes immediately when they exit.

[-- Attachment #1.1.3: Type: text/plain, Size: 325 bytes --]



-- 
 |      Michaël `Micha' Cadilhac   |   Pour les 35-40 ans, l'humour         |
 |         Epita/LRDE Promo 2007   |        c'est une plus-value.           |
 | http://www.lrde.org/~cadilh_m   |           -- Guillaume L.              |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Stefan Monnier]

> In fact, there is already a configuration that would lead to the non
> deletion of the PID from the list:

> - Call to delete_process,
> - Interrupted by a signal before inserting  in deleted_processes_list ,
> - Deletion from  process_alist,

How can this happen?  Are you saying that a signal handler may delete the
process from process_alist?  Which signal handler can do that?

I do think there's a leak but it goes like this:

- catch the SIGCHLD signal
- start processing delete-process, which will add the PID to
  deleted_pid_list even though the signal has already been caught so the pid
  won't get a chance to be removed.

I guess we could check the process's status after adding it to
deleted_pid_list, to catch this case.


        Stefan

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 994 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In fact, there is already a configuration that would lead to the non
>> deletion of the PID from the list:
>
>> - Call to delete_process,
>> - Interrupted by a signal before inserting  in deleted_processes_list ,
>> - Deletion from  process_alist,
>
> How can this happen?  Are you saying that a signal handler may delete the
> process from process_alist?  Which signal handler can do that?

You're right, it's not its job.

> I do think there's a leak but it goes like this:
>
> - catch the SIGCHLD signal
> - start processing delete-process, which will add the PID to
>   deleted_pid_list even though the signal has already been caught so the pid
>   won't get a chance to be removed.

> I guess we could check the process's status after adding it to
> deleted_pid_list, to catch this case.

I think it would be OK. Here's the new (new new) version of the patch.
The kill_process part is not called if the process is already
terminated.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 4516 bytes --]

Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	29 May 2006 11:36:31 -0000
***************
*** 778,783 ****
--- 778,793 ----
    return proc;
  }
  
+ 
+ #ifdef SIGCHLD
+ /* Fdelete_process promises to immediately forget about the process, but in
+    reality, Emacs needs to remember those processes until they have been
+    treated by sigchld_handler; otherwise this handler would consider the
+    process as being synchronous and say that the synchronous process is
+    dead.  */
+ static Lisp_Object deleted_pid_list;
+ #endif
+ 
  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
         doc: /* Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
***************
*** 799,810 ****
      }
    else if (XINT (p->infd) >= 0)
      {
!       Fkill_process (process, Qnil);
!       /* Do this now, since remove_process will make sigchld_handler do nothing.  */
!       p->status
! 	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
!       XSETINT (p->tick, ++process_tick);
!       status_notify (p);
      }
    remove_process (process);
    return Qnil;
--- 809,839 ----
      }
    else if (XINT (p->infd) >= 0)
      {
! #ifdef SIGCHLD
!       Lisp_Object symbol;
! 
!       /* No problem storing the pid here, as it is still in Vprocess_alist.  */
!       deleted_pid_list = Fcons (make_fixnum_or_float (p->pid),
! 				/* GC treated elements set to nil.  */
! 				Fdelq (Qnil, deleted_pid_list));
!       /* If the process has already signaled, remove it from the list.  */
!       if (p->raw_status_new)
! 	update_status (p);
!       symbol = p->status;
!       if (CONSP (p->status))
! 	symbol = XCAR (p->status);
!       if (EQ (symbol, Qsignal) || EQ (symbol, Qexit))
! 	Fdelete (make_fixnum_or_float (p->pid), deleted_pid_list);
!       else
! #endif
! 	{
! 	  Fkill_process (process, Qnil);
! 	  /* Do this now, since remove_process will make sigchld_handler do nothing.  */
! 	  p->status
! 	    = Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
! 	  XSETINT (p->tick, ++process_tick);
! 	  status_notify (p);
! 	}
      }
    remove_process (process);
    return Qnil;
***************
*** 6373,6378 ****
--- 6402,6418 ----
  
        /* Find the process that signaled us, and record its status.  */
  
+ #ifdef SIGCHLD
+       /* The process can have been deleted by Fdelete_process.  */
+       tail = Fmember (make_fixnum_or_float (pid), deleted_pid_list);
+       if (!NILP (tail))
+ 	{
+ 	  Fsetcar (tail, Qnil);
+ 	  goto sigchld_end_of_loop;
+ 	}
+ #endif
+ 
+       /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
        p = 0;
        for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
  	{
***************
*** 6424,6431 ****
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
! 	/* There was no asynchronous process found for that id.  Check
! 	   if we have a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
--- 6464,6471 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
!       /* There was no asynchronous process found for that pid: we have
! 	 a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
***************
*** 6442,6447 ****
--- 6482,6491 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
+ 
+     sigchld_end_of_loop:
+       ;
+ 
        /* On some systems, we must return right away.
  	 If any more processes want to signal us, we will
  	 get another signal.
***************
*** 6843,6848 ****
--- 6887,6895 ----
    FD_SET (0, &input_wait_mask);
  
    Vprocess_alist = Qnil;
+ #ifdef SIGCHLD
+   deleted_pid_list = Qnil;
+ #endif
    for (i = 0; i < MAXDESC; i++)
      {
        chan_process[i] = Qnil;
***************
*** 6981,6986 ****
--- 7028,7036 ----
    staticpro (&Qlast_nonmenu_event);
  
    staticpro (&Vprocess_alist);
+ #ifdef SIGCHLD
+   staticpro (&deleted_pid_list);
+ #endif
  
    DEFVAR_BOOL ("delete-exited-processes", &delete_exited_processes,
  	       doc: /* *Non-nil means delete processes immediately when they exit.

[-- Attachment #1.1.3: Type: text/plain, Size: 323 bytes --]


-- 
 |      Michaël `Micha' Cadilhac   |   Un certain Blaise Pascal             |
 |         Epita/LRDE Promo 2007   |     etc... etc...                      |
 | http://www.lrde.org/~cadilh_m   |   -- Prévert (Les paris stupides)      |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> But it would be cleaner, indeed, to define the whole thing iff SIGCHLD
> is defined.
>
> I modified the patch for that (including in sigchld_handler and so
> because the list is not defined if SIGCHLD is not), and to make the
> insertion in deleted_processes_list before the kill.

Looking at the code once more, I noticed that sigchld_handler is only
installed (i.e. used) if SIGCHLD is defined, but it is defined unconditionally.

So conditioning the entire sigchld_handler in #ifdef SIGCHLD would be much 
cleaner IMO.


And to Eli:

I also saw how MS-Windows handles process termination without SIGCHLD.
Actually, it would do harm if SIGCHLD was ever defined on MS-Windows,
so perhaps we should explicitly add something like this after
all includes:


/* MS-Windows has its own way of detecting process termination.
   It does not normally define SIGCHLD, but just in case...  */
#ifdef WINDOWSNT
#undef SIGCHLD
#endif

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> And to Eli:
>
> I also saw how MS-Windows handles process termination without SIGCHLD.
> Actually, it would do harm if SIGCHLD was ever defined on MS-Windows,
> so perhaps we should explicitly add something like this after
> all includes:
>
>
> /* MS-Windows has its own way of detecting process termination.
>    It does not normally define SIGCHLD, but just in case...  */
> #ifdef WINDOWSNT
> #undef SIGCHLD
> #endif

I seem to remember that Cygwin emulates an awful lot of Unix/Posix
functionality in its DLL.  Others will be more qualified to comment on
this, but the mere use of WINDOWSNT might not necessarily mean that
SIGCHLD can't work: it might be provided by the emulation layer.

Could somebody who actually knows Cygwin by more than heresay fill in
the details and tell whether I am completely wrong here?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: delete-process bug

[Eli Zaretskii]

> From: David Kastrup <dak@gnu.org>
> Date: Mon, 29 May 2006 10:50:42 +0200
> Cc: Agustin Martin <agustin.martin@hispalinux.es>,
> 	=?iso-8859-1?Q?Micha=EBl?= Cadilhac <michael.cadilhac@lrde.org>,
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> > /* MS-Windows has its own way of detecting process termination.
> >    It does not normally define SIGCHLD, but just in case...  */
> > #ifdef WINDOWSNT
> > #undef SIGCHLD
> > #endif
> 
> I seem to remember that Cygwin emulates an awful lot of Unix/Posix
> functionality in its DLL.  Others will be more qualified to comment on
> this, but the mere use of WINDOWSNT might not necessarily mean that
> SIGCHLD can't work: it might be provided by the emulation layer.

Cygwin doesn't define WINDOWSNT when it compiles Emacs.  It mostly
compiles the Unix code, except where you see __CYGWIN__ or CYGWIN in
the sources.

Re: delete-process bug

[Eli Zaretskii]

> From: storm@cua.dk (Kim F. Storm)
> Date: Mon, 29 May 2006 10:22:13 +0200
> Cc: Agustin Martin <agustin.martin@hispalinux.es>,
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> I also saw how MS-Windows handles process termination without SIGCHLD.
> Actually, it would do harm if SIGCHLD was ever defined on MS-Windows,
> so perhaps we should explicitly add something like this after
> all includes:
> 
> 
> /* MS-Windows has its own way of detecting process termination.
>    It does not normally define SIGCHLD, but just in case...  */
> #ifdef WINDOWSNT
> #undef SIGCHLD
> #endif

??? The Windows port already defines SIGCHLD (on src/s/ms-w32.h) and
uses that definition in w32proc.c, in the fragment I've shown and
elsewhere.

Where did you see that defining SIGCHLD on Windows will do harm?  If
it does, we already have that harm.

Re: delete-process bug

[Kim F. Storm]

Eli Zaretskii <eliz@gnu.org> writes:

> ??? The Windows port already defines SIGCHLD (on src/s/ms-w32.h) and
> uses that definition in w32proc.c, in the fragment I've shown and
> elsewhere.
>
> Where did you see that defining SIGCHLD on Windows will do harm?  If
> it does, we already have that harm.

My err!  I didn't study the code well enough.

I now understand that on MS-Windows, the SIGCHLD signal handler is
still called on process termination, but not by the normal signal
mechanism.

So I'll shut up :-)


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Eli Zaretskii]

> Cc: agustin.martin@hispalinux.es,  michael.cadilhac@lrde.org,
> 	  monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> From: storm@cua.dk (Kim F. Storm)
> Date: Mon, 29 May 2006 23:42:22 +0200
> 
> I now understand that on MS-Windows, the SIGCHLD signal handler is
> still called on process termination, but not by the normal signal
> mechanism.

Right.

Re: delete-process bug

[Eli Zaretskii]

> From: storm@cua.dk (Kim F. Storm)
> Date: Sun, 28 May 2006 23:15:58 +0200
> Cc: Agustin Martin <agustin.martin@hispalinux.es>,
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> But I also wonder if any of the async process stuff actually works
> without SIGCHLD ?!?   Are there any systems which support async processes
> but don't have SIGCHLD?

AFAIK, MS-Windows is such a system.  (I didn't track this discussion,
so I don't know if the Windows case is relevant to it.)

Re: delete-process bug

[Kim F. Storm]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: storm@cua.dk (Kim F. Storm)
>> Date: Sun, 28 May 2006 23:15:58 +0200
>> Cc: Agustin Martin <agustin.martin@hispalinux.es>,
>> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
>> 
>> But I also wonder if any of the async process stuff actually works
>> without SIGCHLD ?!?   Are there any systems which support async processes
>> but don't have SIGCHLD?
>
> AFAIK, MS-Windows is such a system.  (I didn't track this discussion,
> so I don't know if the Windows case is relevant to it.)

Well, there is this piece of code (in a context where signo == SIGCHLD):

#if (defined WINDOWSNT \
     || (defined USG && !defined GNU_LINUX \
         && !(defined HPUX && defined WNOHANG)))
#if defined (USG) && ! defined (POSIX_SIGNALS)
      signal (signo, sigchld_handler);
#endif


But... if MS-Windows does not support SIGCHLD (or SIGCLD), how does
emacs detect process termination on MS-Windows??

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 1558 bytes --]

storm@cua.dk (Kim F. Storm) writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: storm@cua.dk (Kim F. Storm)
>>> Date: Sun, 28 May 2006 23:15:58 +0200
>>> Cc: Agustin Martin <agustin.martin@hispalinux.es>,
>>> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
>>> 
>>> But I also wonder if any of the async process stuff actually works
>>> without SIGCHLD ?!?   Are there any systems which support async processes
>>> but don't have SIGCHLD?
>>
>> AFAIK, MS-Windows is such a system.  (I didn't track this discussion,
>> so I don't know if the Windows case is relevant to it.)
>
> Well, there is this piece of code (in a context where signo == SIGCHLD):
>
> #if (defined WINDOWSNT \
>      || (defined USG && !defined GNU_LINUX \
>          && !(defined HPUX && defined WNOHANG)))
> #if defined (USG) && ! defined (POSIX_SIGNALS)
>       signal (signo, sigchld_handler);
> #endif
>
> But... if MS-Windows does not support SIGCHLD (or SIGCLD), how does
> emacs detect process termination on MS-Windows??

Well,  this  I don't  know,  but  on a  second  thought,  it could  be
a callback. However, the word  `SIGCHLD' is never used in the handler,
so I guess it's kind of independent.

-- 
 |      Michaël `Micha' Cadilhac   |   Would someone please DTRT with this  |
 |         Epita/LRDE Promo 2007   |         then ACK?                      |
 | http://www.lrde.org/~cadilh_m   |           -- Richard Stallman          |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Eli Zaretskii]

> Cc: agustin.martin@hispalinux.es,  michael.cadilhac@lrde.org,
> 	  monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> From: storm@cua.dk (Kim F. Storm)
> Date: Mon, 29 May 2006 10:14:22 +0200
> 
> But... if MS-Windows does not support SIGCHLD (or SIGCLD), how does
> emacs detect process termination on MS-Windows??

It uses a Windows specific API to probe for subprocess termination,
and when that tells us that a child process terminated, we call the
signal handler by hand.  The relevant code is in w32proc.c, around
line 1300:

	      else if (sig_handlers[SIGCHLD] != SIG_DFL &&
		       sig_handlers[SIGCHLD] != SIG_IGN)
		{
    #ifdef FULL_DEBUG
		  DebPrint (("select calling SIGCHLD handler for pid %d\n",
			     cp->pid));
    #endif
		  dead_child = cp;
		  sig_handlers[SIGCHLD] (SIGCHLD);
		  dead_child = NULL;
		}
	    }

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 1145 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: agustin.martin@hispalinux.es,  michael.cadilhac@lrde.org,
>> 	  monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> From: storm@cua.dk (Kim F. Storm)
>> Date: Mon, 29 May 2006 10:14:22 +0200
>> 
>> But... if MS-Windows does not support SIGCHLD (or SIGCLD), how does
>> emacs detect process termination on MS-Windows??
>
> It uses a Windows specific API to probe for subprocess termination,
> and when that tells us that a child process terminated, we call the
> signal handler by hand.

So, is  there a case in  which the deleted_pid list  is not necessary,
and in  which we should not  install the mechanism (to  avoid leaks as
well as unnecessary processing) ?

It seems  that the  guard we  put (#ifdef SIGCHLD)  was not  the right
thing to do, isn't it ?

-- 
 |      Michaël `Micha' Cadilhac   |   All your base are belong to us.      |
 |         Epita/LRDE Promo 2007   |     You have no change to survive      |
 | http://www.lrde.org/~cadilh_m   |        make your time, hahaha.         |
 `--  -   JID: micha@amessage.be --'        -- Zero Wings              -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Eli Zaretskii]

> From: michael.cadilhac@lrde.org (=?iso-8859-1?Q?Micha=EBl?= Cadilhac)
> Cc: storm@cua.dk (Kim F. Storm),  agustin.martin@hispalinux.es,  emacs-devel@gnu.org
> Date: Mon, 29 May 2006 22:04:52 +0200
> 
> >> But... if MS-Windows does not support SIGCHLD (or SIGCLD), how does
> >> emacs detect process termination on MS-Windows??
> >
> > It uses a Windows specific API to probe for subprocess termination,
> > and when that tells us that a child process terminated, we call the
> > signal handler by hand.
> 
> So, is  there a case in  which the deleted_pid list  is not necessary,
> and in  which we should not  install the mechanism (to  avoid leaks as
> well as unnecessary processing) ?
> 
> It seems  that the  guard we  put (#ifdef SIGCHLD)  was not  the right
> thing to do, isn't it ?

Sorry, I don't know: I didn't track this thread, so I'm not sure I
even understand the question.

If there's a message or two (as opposed to the whole thread) which I
could read to figure out what are you asking about, please tell what
that message is, and I will try to answer your questions.

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 1590 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> If there's a message or two (as opposed to the whole thread) which I
> could read to figure out what are you asking about, please tell what
> that message is, and I will try to answer your questions.

I will summarize the whole thing:

1) A race condition was found:

delete_process is called  for a process A, a  synchronous process B is
then launched. sigchld_handler is now called for A, but A is not found
in process_alist, so it is considered to be synchronous, and B will be
said to be dead.

2) The chosen solution is the following:

We should remember the list of deleted processes. It's done thanks to
a static variable `deleted_process_list' in process.c.

When the sighandler is called, it will check if the process that
signaled Emacs is in `deleted_process_list'.

3) Now, we want to avoid leaks with this list: we want to know when
   both delete_process and sigchld_handler are called.

AFAICT, both functions are just in a #ifdef subprocesses. But, maybe,
in some systems, sigchld_handler is not used albeit compiled.

The question I asked is : do we have to disable the
`deleted_process_alist' mechanism in some configuration to avoid that
it will be filled but never cleaned ?

-- 
 |      Michaël `Micha' Cadilhac   |  Pour les 35-40 ans, l'humour          |
 |         Epita/LRDE Promo 2007   |       c'est une plus-value.            |
 | http://www.lrde.org/~cadilh_m   |          -- Guillaume L.               |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Eli Zaretskii]

> From: michael.cadilhac@lrde.org (=?iso-8859-1?Q?Micha=EBl?= Cadilhac)
> Cc: agustin.martin@hispalinux.es,  emacs-devel@gnu.org,  storm@cua.dk
> Date: Mon, 29 May 2006 23:42:39 +0200
> 
> I will summarize the whole thing:

Thanks.  I think I understand now.

> AFAICT, both functions are just in a #ifdef subprocesses. But, maybe,
> in some systems, sigchld_handler is not used albeit compiled.
> 
> The question I asked is : do we have to disable the
> `deleted_process_alist' mechanism in some configuration to avoid that
> it will be filled but never cleaned ?

Well, as we established, at least on MS-Windows, sigchld_handler _is_
called, so your fix should work for the Windows build, AFAIU.

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 1032 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: michael.cadilhac@lrde.org (=?iso-8859-1?Q?Micha=EBl?= Cadilhac)
>> Cc: agustin.martin@hispalinux.es,  emacs-devel@gnu.org,  storm@cua.dk
>> Date: Mon, 29 May 2006 23:42:39 +0200
>> 
>> I will summarize the whole thing:
>
> Thanks.  I think I understand now.
>
>> AFAICT, both functions are just in a #ifdef subprocesses. But, maybe,
>> in some systems, sigchld_handler is not used albeit compiled.
>> 
>> The question I asked is : do we have to disable the
>> `deleted_process_alist' mechanism in some configuration to avoid that
>> it will be filled but never cleaned ?
>
> Well, as we established, at least on MS-Windows, sigchld_handler _is_
> called, so your fix should work for the Windows build, AFAIU.

So, AFAIU too, if delete_process can be called (protected by #ifdef
subprocesses), sigchld_handler can be too.

Patch updated to remove #ifdef SIGCHLD guards attached.

It addresses all the problems we've seen so far, and seems to work (AFAICT). 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 4376 bytes --]

Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	29 May 2006 22:26:17 -0000
***************
*** 778,783 ****
--- 778,791 ----
    return proc;
  }
  
+ 
+ /* Fdelete_process promises to immediately forget about the process, but in
+    reality, Emacs needs to remember those processes until they have been
+    treated by sigchld_handler; otherwise this handler would consider the
+    process as being synchronous and say that the synchronous process is
+    dead.  */
+ static Lisp_Object deleted_pid_list;
+ 
  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
         doc: /* Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
***************
*** 799,810 ****
      }
    else if (XINT (p->infd) >= 0)
      {
!       Fkill_process (process, Qnil);
!       /* Do this now, since remove_process will make sigchld_handler do nothing.  */
!       p->status
! 	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
!       XSETINT (p->tick, ++process_tick);
!       status_notify (p);
      }
    remove_process (process);
    return Qnil;
--- 807,835 ----
      }
    else if (XINT (p->infd) >= 0)
      {
!       Lisp_Object symbol;
! 
!       /* No problem storing the pid here, as it is still in Vprocess_alist.  */
!       deleted_pid_list = Fcons (make_fixnum_or_float (p->pid),
! 				/* GC treated elements set to nil.  */
! 				Fdelq (Qnil, deleted_pid_list));
!       /* If the process has already signaled, remove it from the list.  */
!       if (p->raw_status_new)
! 	update_status (p);
!       symbol = p->status;
!       if (CONSP (p->status))
! 	symbol = XCAR (p->status);
!       if (EQ (symbol, Qsignal) || EQ (symbol, Qexit))
! 	Fdelete (make_fixnum_or_float (p->pid), deleted_pid_list);
!       else
! 	{
! 	  Fkill_process (process, Qnil);
! 	  /* Do this now, since remove_process will make sigchld_handler do nothing.  */
! 	  p->status
! 	    = Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
! 	  XSETINT (p->tick, ++process_tick);
! 	  status_notify (p);
! 	}
      }
    remove_process (process);
    return Qnil;
***************
*** 6373,6378 ****
--- 6398,6412 ----
  
        /* Find the process that signaled us, and record its status.  */
  
+       /* The process can have been deleted by Fdelete_process.  */
+       tail = Fmember (make_fixnum_or_float (pid), deleted_pid_list);
+       if (!NILP (tail))
+ 	{
+ 	  Fsetcar (tail, Qnil);
+ 	  goto sigchld_end_of_loop;
+ 	}
+ 
+       /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
        p = 0;
        for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
  	{
***************
*** 6424,6431 ****
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
! 	/* There was no asynchronous process found for that id.  Check
! 	   if we have a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
--- 6458,6465 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
!       /* There was no asynchronous process found for that pid: we have
! 	 a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
***************
*** 6442,6447 ****
--- 6476,6485 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
+ 
+     sigchld_end_of_loop:
+       ;
+ 
        /* On some systems, we must return right away.
  	 If any more processes want to signal us, we will
  	 get another signal.
***************
*** 6843,6848 ****
--- 6881,6887 ----
    FD_SET (0, &input_wait_mask);
  
    Vprocess_alist = Qnil;
+   deleted_pid_list = Qnil;
    for (i = 0; i < MAXDESC; i++)
      {
        chan_process[i] = Qnil;
***************
*** 6981,6986 ****
--- 7020,7026 ----
    staticpro (&Qlast_nonmenu_event);
  
    staticpro (&Vprocess_alist);
+   staticpro (&deleted_pid_list);
  
    DEFVAR_BOOL ("delete-exited-processes", &delete_exited_processes,
  	       doc: /* *Non-nil means delete processes immediately when they exit.

[-- Attachment #1.1.3: Type: text/plain, Size: 323 bytes --]


-- 
 |      Michaël `Micha' Cadilhac   |    Le copillage-collage                |
 |         Epita/LRDE Promo 2007   |       tue le programmeur.              |
 | http://www.lrde.org/~cadilh_m   |           -- Dictons LRDE              |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> It seems  that the  guard we  put (#ifdef SIGCHLD)  was not  the right
> thing to do, isn't it ?

I promised Eli to shut up, but it once again occurred to me that the
sigchld_handler is only called _IF_ SIGCHLD is defined.

That's true even on MS-Windows (which does define SIGCHLD), although
it uses an indirect method to call the signal handler (see w32proc.c).

Since the sigchld_handler is only installed if SIGCHLD is defined,
it will only ever be called (to cleanup deleted_pid_list) if
SIGCHLD is defined.

So it still seems wrong to put anything on deleted_pid_list unless
SIGCHLD is defined ...

And sigchld_handler should be conditioned by #ifdef SIGCHLD.


Sorry that I'm going round in circles (perhaps my brain in an infinite loop :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 773 bytes --]

storm@cua.dk (Kim F. Storm) writes:

> michael.cadilhac@lrde.org (Michaël Cadilhac) writes:
>
>> It seems  that the  guard we  put (#ifdef SIGCHLD)  was not  the right
>> thing to do, isn't it ?
>
> I promised Eli to shut up

Please don't  :-)

> but it once again occurred to me that the sigchld_handler is only
> called _IF_ SIGCHLD is defined.

> Since the sigchld_handler is only installed if SIGCHLD is defined,
> it will only ever be called (to cleanup deleted_pid_list) if
> SIGCHLD is defined.

> So it still seems wrong to put anything on deleted_pid_list unless
> SIGCHLD is defined ...

> And sigchld_handler should be conditioned by #ifdef SIGCHLD.

Aaaand here comes the new new new (super better) new great patch
v3923.3 (beta 1) !


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 4919 bytes --]

Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	30 May 2006 12:35:10 -0000
***************
*** 778,783 ****
--- 778,793 ----
    return proc;
  }
  
+ 
+ #ifdef SIGCHLD
+ /* Fdelete_process promises to immediately forget about the process, but in
+    reality, Emacs needs to remember those processes until they have been
+    treated by sigchld_handler; otherwise this handler would consider the
+    process as being synchronous and say that the synchronous process is
+    dead.  */
+ static Lisp_Object deleted_pid_list;
+ #endif
+ 
  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
         doc: /* Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
***************
*** 799,810 ****
      }
    else if (XINT (p->infd) >= 0)
      {
!       Fkill_process (process, Qnil);
!       /* Do this now, since remove_process will make sigchld_handler do nothing.  */
!       p->status
! 	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
!       XSETINT (p->tick, ++process_tick);
!       status_notify (p);
      }
    remove_process (process);
    return Qnil;
--- 809,839 ----
      }
    else if (XINT (p->infd) >= 0)
      {
! #ifdef SIGCHLD
!       Lisp_Object symbol;
! 
!       /* No problem storing the pid here, as it is still in Vprocess_alist.  */
!       deleted_pid_list = Fcons (make_fixnum_or_float (p->pid),
! 				/* GC treated elements set to nil.  */
! 				Fdelq (Qnil, deleted_pid_list));
!       /* If the process has already signaled, remove it from the list.  */
!       if (p->raw_status_new)
! 	update_status (p);
!       symbol = p->status;
!       if (CONSP (p->status))
! 	symbol = XCAR (p->status);
!       if (EQ (symbol, Qsignal) || EQ (symbol, Qexit))
! 	Fdelete (make_fixnum_or_float (p->pid), deleted_pid_list);
!       else
! #endif
! 	{
! 	  Fkill_process (process, Qnil);
! 	  /* Do this now, since remove_process will make sigchld_handler do nothing.  */
! 	  p->status
! 	    = Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
! 	  XSETINT (p->tick, ++process_tick);
! 	  status_notify (p);
! 	}
      }
    remove_process (process);
    return Qnil;
***************
*** 6316,6321 ****
--- 6345,6351 ----
     ** Malloc WARNING: This should never call malloc either directly or
     indirectly; if it does, that is a bug  */
  
+ #ifdef SIGCHLD
  SIGTYPE
  sigchld_handler (signo)
       int signo;
***************
*** 6373,6378 ****
--- 6403,6417 ----
  
        /* Find the process that signaled us, and record its status.  */
  
+       /* The process can have been deleted by Fdelete_process.  */
+       tail = Fmember (make_fixnum_or_float (pid), deleted_pid_list);
+       if (!NILP (tail))
+ 	{
+ 	  Fsetcar (tail, Qnil);
+ 	  goto sigchld_end_of_loop;
+ 	}
+ 
+       /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
        p = 0;
        for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
  	{
***************
*** 6424,6431 ****
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
! 	/* There was no asynchronous process found for that id.  Check
! 	   if we have a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
--- 6463,6470 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
!       /* There was no asynchronous process found for that pid: we have
! 	 a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
***************
*** 6442,6447 ****
--- 6481,6490 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
+ 
+     sigchld_end_of_loop:
+       ;
+ 
        /* On some systems, we must return right away.
  	 If any more processes want to signal us, we will
  	 get another signal.
***************
*** 6458,6463 ****
--- 6501,6507 ----
  #endif /* USG, but not HPUX with WNOHANG */
      }
  }
+ #endif /* SIGCHLD */
  \f
  
  static Lisp_Object
***************
*** 6843,6848 ****
--- 6887,6895 ----
    FD_SET (0, &input_wait_mask);
  
    Vprocess_alist = Qnil;
+ #ifdef SIGCHLD
+   deleted_pid_list = Qnil;
+ #endif
    for (i = 0; i < MAXDESC; i++)
      {
        chan_process[i] = Qnil;
***************
*** 6981,6986 ****
--- 7028,7036 ----
    staticpro (&Qlast_nonmenu_event);
  
    staticpro (&Vprocess_alist);
+ #ifdef SIGCHLD
+   staticpro (&deleted_pid_list);
+ #endif
  
    DEFVAR_BOOL ("delete-exited-processes", &delete_exited_processes,
  	       doc: /* *Non-nil means delete processes immediately when they exit.

[-- Attachment #1.1.3: Type: text/plain, Size: 510 bytes --]


> Sorry that I'm going round in circles (perhaps my brain in an
> infinite loop :-)

No problem. Hope you will not need a KILL signal in your brain, for
the sake of your (?) CHLD.

-- 
 |      Michaël `Micha' Cadilhac   |  Mieux vaut se taire                   |
 |         Epita/LRDE Promo 2007   |   Que de parler trop fort.             |
 | http://www.lrde.org/~cadilh_m   |           -- As de trèfle              |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> Aaaand here comes the new new new (super better) new great patch
> v3923.3 (beta 1) !

Looks really fine now.  

Can you write a ChangeLog entry for it, so we can install it.


>> Sorry that I'm going round in circles (perhaps my brain in an
>> infinite loop :-)
>
> No problem. Hope you will not need a KILL signal in your brain, for
> the sake of your (?) CHLD.

I don't know if GDB can be used to peek around before the
final KILL signal to make a core dump, in case anybody
need to do some post-mortem analysis ...

Anyway, I've just sent the looping process a SIGSTOP --  
so I can wake it up again in case we need another round
through the loop ;-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 319 bytes --]

storm@cua.dk (Kim F. Storm) writes:

> michael.cadilhac@lrde.org (Michaël Cadilhac) writes:
>
>> Aaaand here comes the new new new (super better) new great patch
>> v3923.3 (beta 1) !
>
> Looks really fine now.  
>
> Can you write a ChangeLog entry for it, so we can install it.

The whole patch follows:


[-- Attachment #1.1.2: process.patch --]
[-- Type: text/x-patch, Size: 6061 bytes --]

Index: src/process.c
===================================================================
RCS file: /sources/emacs/emacs/src/process.c,v
retrieving revision 1.481
diff -c -r1.481 process.c
*** src/process.c	8 May 2006 05:19:42 -0000	1.481
--- src/process.c	30 May 2006 15:04:55 -0000
***************
*** 778,783 ****
--- 778,793 ----
    return proc;
  }
  
+ 
+ #ifdef SIGCHLD
+ /* Fdelete_process promises to immediately forget about the process, but in
+    reality, Emacs needs to remember those processes until they have been
+    treated by sigchld_handler; otherwise this handler would consider the
+    process as being synchronous and say that the synchronous process is
+    dead.  */
+ static Lisp_Object deleted_pid_list;
+ #endif
+ 
  DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
         doc: /* Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
***************
*** 799,810 ****
      }
    else if (XINT (p->infd) >= 0)
      {
!       Fkill_process (process, Qnil);
!       /* Do this now, since remove_process will make sigchld_handler do nothing.  */
!       p->status
! 	= Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
!       XSETINT (p->tick, ++process_tick);
!       status_notify (p);
      }
    remove_process (process);
    return Qnil;
--- 809,839 ----
      }
    else if (XINT (p->infd) >= 0)
      {
! #ifdef SIGCHLD
!       Lisp_Object symbol;
! 
!       /* No problem storing the pid here, as it is still in Vprocess_alist.  */
!       deleted_pid_list = Fcons (make_fixnum_or_float (p->pid),
! 				/* GC treated elements set to nil.  */
! 				Fdelq (Qnil, deleted_pid_list));
!       /* If the process has already signaled, remove it from the list.  */
!       if (p->raw_status_new)
! 	update_status (p);
!       symbol = p->status;
!       if (CONSP (p->status))
! 	symbol = XCAR (p->status);
!       if (EQ (symbol, Qsignal) || EQ (symbol, Qexit))
! 	Fdelete (make_fixnum_or_float (p->pid), deleted_pid_list);
!       else
! #endif
! 	{
! 	  Fkill_process (process, Qnil);
! 	  /* Do this now, since remove_process will make sigchld_handler do nothing.  */
! 	  p->status
! 	    = Fcons (Qsignal, Fcons (make_number (SIGKILL), Qnil));
! 	  XSETINT (p->tick, ++process_tick);
! 	  status_notify (p);
! 	}
      }
    remove_process (process);
    return Qnil;
***************
*** 6316,6321 ****
--- 6345,6351 ----
     ** Malloc WARNING: This should never call malloc either directly or
     indirectly; if it does, that is a bug  */
  
+ #ifdef SIGCHLD
  SIGTYPE
  sigchld_handler (signo)
       int signo;
***************
*** 6373,6378 ****
--- 6403,6417 ----
  
        /* Find the process that signaled us, and record its status.  */
  
+       /* The process can have been deleted by Fdelete_process.  */
+       tail = Fmember (make_fixnum_or_float (pid), deleted_pid_list);
+       if (!NILP (tail))
+ 	{
+ 	  Fsetcar (tail, Qnil);
+ 	  goto sigchld_end_of_loop;
+ 	}
+ 
+       /* Otherwise, if it is asynchronous, it is in Vprocess_alist.  */
        p = 0;
        for (tail = Vprocess_alist; GC_CONSP (tail); tail = XCDR (tail))
  	{
***************
*** 6424,6431 ****
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
! 	/* There was no asynchronous process found for that id.  Check
! 	   if we have a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
--- 6463,6470 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
!       /* There was no asynchronous process found for that pid: we have
! 	 a synchronous process.  */
        else
  	{
  	  synch_process_alive = 0;
***************
*** 6442,6447 ****
--- 6481,6490 ----
  	    EMACS_SET_SECS_USECS (*input_available_clear_time, 0, 0);
  	}
  
+ 
+     sigchld_end_of_loop:
+       ;
+ 
        /* On some systems, we must return right away.
  	 If any more processes want to signal us, we will
  	 get another signal.
***************
*** 6458,6463 ****
--- 6501,6507 ----
  #endif /* USG, but not HPUX with WNOHANG */
      }
  }
+ #endif /* SIGCHLD */
  \f
  
  static Lisp_Object
***************
*** 6843,6848 ****
--- 6887,6895 ----
    FD_SET (0, &input_wait_mask);
  
    Vprocess_alist = Qnil;
+ #ifdef SIGCHLD
+   deleted_pid_list = Qnil;
+ #endif
    for (i = 0; i < MAXDESC; i++)
      {
        chan_process[i] = Qnil;
***************
*** 6981,6986 ****
--- 7028,7036 ----
    staticpro (&Qlast_nonmenu_event);
  
    staticpro (&Vprocess_alist);
+ #ifdef SIGCHLD
+   staticpro (&deleted_pid_list);
+ #endif
  
    DEFVAR_BOOL ("delete-exited-processes", &delete_exited_processes,
  	       doc: /* *Non-nil means delete processes immediately when they exit.
Index: src/ChangeLog
===================================================================
RCS file: /sources/emacs/emacs/src/ChangeLog,v
retrieving revision 1.5110
diff -c -r1.5110 ChangeLog
*** src/ChangeLog	30 May 2006 04:33:52 -0000	1.5110
--- src/ChangeLog	30 May 2006 15:04:56 -0000
***************
*** 1,3 ****
--- 1,16 ----
+ 2006-05-30  Michaël Cadilhac  <michael.cadilhac@lrde.org>
+ 
+ 	* process.c (deleted_pid_list): Add a static variable to store the pids
+ 	of the deleted processes. Declare it only if SIGCHLD is defined.
+ 	(init_process): Initialize it.
+ 	(syms_of_process): Staticpro it.
+ 	(Fdelete_process): Add the deleted process to it. Check after the
+ 	addition and before the kill if the process is already stopped, in
+ 	which case it is deleted from the list and not killed.
+ 	(sigchld_handler): Define it only if SIGCHLD is. Search the process
+ 	that signaled Emacs in `deleted_pid_list' before `Vprocess_alist'.
+ 	Original idea by Stefan Monnier <monnier@iro.umontreal.ca>.
+ 	
  2006-05-30  Richard Stallman  <rms@gnu.org>
  
  	* coding.c (Ffind_operation_coding_system): Doc fix.

[-- Attachment #1.1.3: Type: text/plain, Size: 886 bytes --]


>> No problem. Hope you will not need a KILL signal in your brain, for
>> the sake of your (?) CHLD.
>
> I don't know if GDB can be used to peek around before the
> final KILL signal to make a core dump, in case anybody
> need to do some post-mortem analysis ...

Mmmh... Wondering if someone HAS to see you after the KILL signal to
avoid being a zombie.

> Anyway, I've just sent the looping process a SIGSTOP --  
> so I can wake it up again in case we need another round
> through the loop ;-)

We shall debug that, but after the release. :-)

-- 
 |      Michaël `Micha' Cadilhac   |  Mieux vaut se taire                   |
 |         Epita/LRDE Promo 2007   |   Que de parler trop fort.             |
 | http://www.lrde.org/~cadilh_m   |           -- As de trèfle              |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> The whole patch follows:

Thanks.  Installed.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 896 bytes --]

storm@cua.dk (Kim F. Storm) writes:

> michael.cadilhac@lrde.org (Michaël Cadilhac) writes:
>
>> The whole patch follows:
>
> Thanks.  Installed.

Thanks!

Now that this bug is fixed, Agustin, can you check (a third time) if
the first patch is OK? (Copy of the message follows)

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> I was wondering why ispell-kill-process took so much time to
> execute.
>
> This function does the following:
>   - Send EOF to ispell,
>   - Read its output if there is, timeout to 1 sec,
>   - Kill the process if it's still not,
>   - Wait for it to be really killed (sleeping for 0.25 sec between checks).

Oh, and BTW, why is the wait_for_termination function not provided to
the user?

> I don't want to be ... rude, I'm really a pacifist actually, but why
> not just delete-process it ?

> I propose the following change:


[-- Attachment #1.1.2: ispell.patch --]
[-- Type: text/x-patch, Size: 1960 bytes --]

Index: lisp/ChangeLog
===================================================================
RCS file: /sources/emacs/emacs/lisp/ChangeLog,v
retrieving revision 1.9586
diff -c -B -b -r1.9586 ChangeLog
*** lisp/ChangeLog	23 May 2006 11:23:25 -0000	1.9586
--- lisp/ChangeLog	23 May 2006 18:17:41 -0000
***************
*** 1,3 ****
--- 1,9 ----
+ 2006-05-23  Michaël Cadilhac  <michael.cadilhac@lrde.org>
+ 
+ 	* textmodes/ispell.el (ispell-kill-ispell): If ispell has been
+ 	launched asynchronously, delete its process instead of being
+ 	cool.
+ 
  2006-05-23  Thien-Thi Nguyen  <ttn@gnu.org>
  
  	* emacs-lisp/ewoc.el (ewoc-delete): New function.
Index: lisp/textmodes/ispell.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/textmodes/ispell.el,v
retrieving revision 1.199
diff -c -B -b -r1.199 ispell.el
*** lisp/textmodes/ispell.el	21 May 2006 20:25:43 -0000	1.199
--- lisp/textmodes/ispell.el	23 May 2006 18:17:41 -0000
***************
*** 2572,2586 ****
        (or no-error
  	  (error "There is no ispell process running!"))
      (if ispell-async-processp
! 	(progn
! 	  (process-send-eof ispell-process)
! 	  (if (eq (ispell-process-status) 'run)
! 	      (ispell-accept-output 1))
! 	  (if (eq (ispell-process-status) 'run)
! 	      (kill-process ispell-process))
! 	  (while (not (or (eq (ispell-process-status) 'exit)
! 			  (eq (ispell-process-status) 'signal)))
! 	    (sleep-for 0.25)))
        ;; synchronous processes
        (ispell-send-string "\n")		; make sure side effects occurred.
        (kill-buffer ispell-output-buffer)
--- 2572,2578 ----
        (or no-error
  	  (error "There is no ispell process running!"))
      (if ispell-async-processp
! 	(delete-process ispell-process)
        ;; synchronous processes
        (ispell-send-string "\n")		; make sure side effects occurred.
        (kill-buffer ispell-output-buffer)

[-- Attachment #1.1.3: Type: text/plain, Size: 325 bytes --]



-- 
 |      Michaël `Micha' Cadilhac   |  Mieux vaut se taire                   |
 |         Epita/LRDE Promo 2007   |   Que de parler trop fort.             |
 | http://www.lrde.org/~cadilh_m   |           -- As de trèfle              |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

>
> Oh, and BTW, why is the wait_for_termination function not provided to
> the user?

Why?

I think you can use accept-process-output in a loop to wait for the
death of a specific process.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 1399 bytes --]

storm@cua.dk (Kim F. Storm) writes:

> michael.cadilhac@lrde.org (Michaël Cadilhac) writes:
>
>>
>> Oh, and BTW, why is the wait_for_termination function not provided to
>> the user?
>
> Why?
>
> I think you can use accept-process-output in a loop to wait for the
> death of a specific process.

Oh, well, just that the code in ispell I propose to change used is
following:

	  (process-send-eof ispell-process)
 	  (if (eq (ispell-process-status) 'run)
 	      (ispell-accept-output 1))
 	  (if (eq (ispell-process-status) 'run)
 	      (kill-process ispell-process))
 	  (while (not (or (eq (ispell-process-status) 'exit)
 			  (eq (ispell-process-status) 'signal)))
 	    (sleep-for 0.25)))

ispell-accept-output just being a wrapper around
accept-process-output. It used a timeout of 1 sec, but `process' was
bound in accept-process-output. However, as `process' is bound, ...

« Non-nil arg process means do not return until some output has been
  received from process. »

So, was the timeout useful or at least meaningful in that case?

-- 
 |      Michaël `Micha' Cadilhac   |  Would someone please DTRT with this,  |
 |         Epita/LRDE Promo 2007   |        then ACK?                       |
 | http://www.lrde.org/~cadilh_m   |          -- Richard Stallman           |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> Oh, well, just that the code in ispell I propose to change used is
> following:
>
> 	  (process-send-eof ispell-process)
>  	  (if (eq (ispell-process-status) 'run)
>  	      (ispell-accept-output 1))
>  	  (if (eq (ispell-process-status) 'run)
>  	      (kill-process ispell-process))
>  	  (while (not (or (eq (ispell-process-status) 'exit)
>  			  (eq (ispell-process-status) 'signal)))
>  	    (sleep-for 0.25)))
>
> So, was the timeout useful or at least meaningful in that case?

Maybe as a safety valve in case the process didn't terminate after being fed the EOF.

Using delete-process as you suggested seems much better, indeed.


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: delete-process bug

[Agustin Martin]

On Thu, Jun 01, 2006 at 04:20:43PM +0200, Michaël Cadilhac wrote:
> storm@cua.dk (Kim F. Storm) writes:
> 
> > michael.cadilhac@lrde.org (Michaël Cadilhac) writes:
> >
> >> The whole patch follows:
> >
> > Thanks.  Installed.
> 
> Thanks!
> 
> Now that this bug is fixed, Agustin, can you check (a third time) if
> the first patch is OK? (Copy of the message follows)

Tested with 20060524 snapshot with your ispell.el patch and today's last
installed process.c (with the last patch)

> 1) (original test) Quick switching between two flyspellized buffers
>    that have different dictionaries,

Works very well

> 2) 
>
> (let (processes)
>   (dotimes (i 100)
>      (push (start-process "bug" nil "/tmp/sleep") processes))
>    (dotimes (i 50)
>      (if (zerop (mod i 2))
>   	(delete-process (pop processes))
>        (kill-process (pop processes))))
>    (message "%S" (call-process "/tmp/sleep"))
>    (dotimes (i 50)
>      (delete-process (pop processes)))
>    (message "%S" (call-process "/tmp/sleep")))

I am getting 

if: Process bug<98> is not active

here,

-- 
Agustin

Re: delete-process bug

[Michaël Cadilhac]

[-- Attachment #1.1: Type: text/plain, Size: 1160 bytes --]

Agustin Martin <agustin.martin@hispalinux.es> writes:

>> 1) (original test) Quick switching between two flyspellized buffers
>>    that have different dictionaries,
>
> Works very well

Fine!

>> 2) 
>>
>> (let (processes)
>>   (dotimes (i 100)
>>      (push (start-process "bug" nil "/tmp/sleep") processes))
>>    (dotimes (i 50)
>>      (if (zerop (mod i 2))
>>   	(delete-process (pop processes))
>>        (kill-process (pop processes))))
>>    (message "%S" (call-process "/tmp/sleep"))
>>    (dotimes (i 50)
>>      (delete-process (pop processes)))
>>    (message "%S" (call-process "/tmp/sleep")))
>
> I am getting 
>
> if: Process bug<98> is not active

This is no bug in fact. The test I proposed is not really good, it
could try to kill dead processes, and it does. :-)

Thank you very much !

-- 
 |      Michaël `Micha' Cadilhac   |  Un certain Blaise Pascal              |
 |         Epita/LRDE Promo 2007   |    etc... etc...                       |
 | http://www.lrde.org/~cadilh_m   |  -- Prévert (Les paris stupides)       |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: delete-process bug

[Agustin Martin]

On Sun, May 28, 2006 at 10:26:43PM +0200, Michaël Cadilhac wrote:
> (Final ?) patch attached.
> 
> Agustin, can you check that you can't trigger the bug anymore?
> 

Tested in my system with the (Final ?)+2 (The new-new-new patch), not the
one you have just sent with '#ifdef SIGCHLD' removed but the previous one.
I tested repeated switching between flyspellized buffers. Running
emacs-snapshot + your patches in a Debian unstable box seems to work very
well. Thanks a lot.

-- 
Agustin

Re: delete-process bug (was: Ispell loads dict twice.)

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> You'll have, as a result, the message « Killed » ; but the process
> called by call-process is _not actually killed_, it's the previous
> (deleted) one that was.

Thank you very much for debugging this!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Ispell loads dict twice.

[Michaël Cadilhac]

[-- Attachment #1.1.1: Type: text/plain, Size: 782 bytes --]


Can someone please install the following change, as it has been
reviewed by the concerned people?

Thanks.

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> I was wondering why ispell-kill-process took so much time to
> execute.
>
> This function does the following:
> - Send EOF to ispell,
> - Read its output if there is, timeout to 1 sec,
> - Kill the process if it's still not,
> - Wait for it to be really killed (sleeping for 0.25 sec between checks).
>
> I don't want to be ... rude, I'm really a pacifist actually, but why
> not just delete-process it ?
>
> I often have some flyspellized buffers in English and some others in
> French, the time I have to wait on every C-x o is kind of
> disturbing...
>
> I propose the following change:


[-- Attachment #1.1.2: ispell.patch --]
[-- Type: text/x-patch, Size: 2639 bytes --]

Index: lisp/ChangeLog
===================================================================
RCS file: /sources/emacs/emacs/lisp/ChangeLog,v
retrieving revision 1.9661
diff -c -r1.9661 ChangeLog
*** lisp/ChangeLog	1 Jun 2006 06:42:07 -0000	1.9661
--- lisp/ChangeLog	6 Jun 2006 20:42:02 -0000
***************
*** 1,3 ****
--- 1,11 ----
+ 2006-06-06  Michaël Cadilhac  <michael.cadilhac@lrde.org>
+ 
+ 	* textmodes/ispell.el (ispell-kill-ispell): If ispell has been
+ 	launched asynchronously, delete its process instead of being
+ 	cool.
+ 	(ispell-async-processp): Check for `delete-process' existence
+ 	instead of `kill-process' one for consistency.
+ 
  2006-06-01  Jan Djärv  <jan.h.d@swipnet.se>
  
  	* term/x-win.el: Change x-menu-bar-start to menu-bar-open.
Index: lisp/textmodes/ispell.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/textmodes/ispell.el,v
retrieving revision 1.199
diff -c -r1.199 ispell.el
*** lisp/textmodes/ispell.el	21 May 2006 20:25:43 -0000	1.199
--- lisp/textmodes/ispell.el	6 Jun 2006 20:42:03 -0000
***************
*** 865,871 ****
  (defvar ispell-process nil
    "The process object for Ispell.")
  
! (defvar ispell-async-processp (and (fboundp 'kill-process)
  				   (fboundp 'process-send-string)
  				   (fboundp 'accept-process-output)
  				   ;;(fboundp 'start-process)
--- 865,871 ----
  (defvar ispell-process nil
    "The process object for Ispell.")
  
! (defvar ispell-async-processp (and (fboundp 'delete-process)
  				   (fboundp 'process-send-string)
  				   (fboundp 'accept-process-output)
  				   ;;(fboundp 'start-process)
***************
*** 2572,2586 ****
        (or no-error
  	  (error "There is no ispell process running!"))
      (if ispell-async-processp
! 	(progn
! 	  (process-send-eof ispell-process)
! 	  (if (eq (ispell-process-status) 'run)
! 	      (ispell-accept-output 1))
! 	  (if (eq (ispell-process-status) 'run)
! 	      (kill-process ispell-process))
! 	  (while (not (or (eq (ispell-process-status) 'exit)
! 			  (eq (ispell-process-status) 'signal)))
! 	    (sleep-for 0.25)))
        ;; synchronous processes
        (ispell-send-string "\n")		; make sure side effects occurred.
        (kill-buffer ispell-output-buffer)
--- 2572,2578 ----
        (or no-error
  	  (error "There is no ispell process running!"))
      (if ispell-async-processp
! 	(delete-process ispell-process)
        ;; synchronous processes
        (ispell-send-string "\n")		; make sure side effects occurred.
        (kill-buffer ispell-output-buffer)

[-- Attachment #1.1.3: Type: text/plain, Size: 323 bytes --]


-- 
 |      Michaël `Micha' Cadilhac   |  La culture c'est comme la confiture,  |
 |         Epita/LRDE Promo 2007   |      c'est meilleur avec du pain.      |
 | http://www.lrde.org/~cadilh_m   |           -- MOI59                     |
 `--  -   JID: micha@amessage.be --'                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Ispell loads dict twice.

[Kim F. Storm]

michael.cadilhac@lrde.org (Michaël Cadilhac) writes:

> Can someone please install the following change, as it has been
> reviewed by the concerned people?

Done.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk