Carbon (macOS) port asks about killing stderr buffer

[David Edmondson]

After the changes to use `make-process', the Carbon port of emacs on
macOS (often referred to as emacs-mac or the railwaycat port) will ask
about killing the stderr buffer after any `notmuch-search':

Debugger entered--Lisp error: (quit)
  yes-or-no-p("Buffer \" *notmuch-stderr*-839121\" has a running process; kill it? ")
  process-kill-buffer-query-function()
  kill-buffer(#<buffer  *notmuch-stderr*-839121>)
  notmuch-start-notmuch-sentinel(#<process notmuch-search> "finished\n")

A quick look at the implementation of `make-process' in the Carbon port
didn't reveal anything obvious to me. This mostly seems like a race -
whether emacs has decided that the process associated with the stderr
buffer is dead or not when we call `kill-buffer'. Is any ordering
guaranteed by the implementation?

I _think_ that have also seen the same problem when asynchronous address
harvesting is happening for completion on the default NextStep port for
macOS, but haven't been able to reliably reproduce it.

dme.
--
I got a girlfriend that's better than that.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

[David Edmondson]

On Thursday, 2017-10-26 at 12:32:17 +0100, David Edmondson wrote:

> After the changes to use `make-process', the Carbon port of emacs on
> macOS (often referred to as emacs-mac or the railwaycat port) will ask
> about killing the stderr buffer after any `notmuch-search':
>
> Debugger entered--Lisp error: (quit)
>   yes-or-no-p("Buffer \" *notmuch-stderr*-839121\" has a running process; kill it? ")
>   process-kill-buffer-query-function()
>   kill-buffer(#<buffer  *notmuch-stderr*-839121>)
>   notmuch-start-notmuch-sentinel(#<process notmuch-search> "finished\n")

A hack-and-slash patch for this that works for me:

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 010be454..7c0faee4 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -909,7 +909,7 @@ invoke `set-process-sentinel' directly on the returned process,
 as that will interfere with the handling of stderr and the exit
 status."
 
-  (let (err-file err-buffer proc
+  (let (err-file err-buffer proc err-proc
  ;; Find notmuch using Emacs' `exec-path'
  (command (or (executable-find notmuch-command)
      (error "Command not found: %s" notmuch-command))))
@@ -926,11 +926,13 @@ status."
       :buffer buffer
       :command (cons command args)
       :connection-type 'pipe
-      :stderr err-buffer))
+      :stderr err-buffer)
+ err-proc (get-buffer-process err-buffer))
   (process-put proc 'err-buffer err-buffer)
-  ;; Silence "Process NAME stderr finished" in stderr by adding a
-  ;; no-op sentinel to the fake stderr process object
-  (set-process-sentinel (get-buffer-process err-buffer) #'ignore))
+
+  (process-put err-proc 'err-file err-file)
+  (process-put err-proc 'err-buffer err-buffer)
+  (set-process-sentinel err-proc #'notmuch-start-notmuch-error-sentinel))
 
       ;; On Emacs versions before 25, there is no way to capture
       ;; stdout and stderr separately for asynchronous processes, or
@@ -990,9 +992,14 @@ status."
        ;; Emacs behaves strangely if an error escapes from a sentinel,
        ;; so turn errors into messages.
        (message "%s" (error-message-string err))))
-    (when err-buffer (kill-buffer err-buffer))
     (when err-file (ignore-errors (delete-file err-file)))))
 
+(defun notmuch-start-notmuch-error-sentinel (proc event)
+  (let* ((err-file (process-get proc 'err-file))
+ (err-buffer (or (process-get proc 'err-buffer)
+ (find-file-noselect err-file))))
+    (when err-buffer (kill-buffer err-buffer))))
+
 ;; This variable is used only buffer local, but it needs to be
 ;; declared globally first to avoid compiler warnings.
 (defvar notmuch-show-process-crypto nil)

dme.
--
Why stay in college? Why go to night school?
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

[David Edmondson]

On Thursday, 2017-10-26 at 12:32:17 +0100, David Edmondson wrote:

> After the changes to use `make-process', the Carbon port of emacs on
> macOS (often referred to as emacs-mac or the railwaycat port) will ask
> about killing the stderr buffer after any `notmuch-search':
>
> Debugger entered--Lisp error: (quit)
>   yes-or-no-p("Buffer \" *notmuch-stderr*-839121\" has a running process; kill it? ")
>   process-kill-buffer-query-function()
>   kill-buffer(#<buffer  *notmuch-stderr*-839121>)
>   notmuch-start-notmuch-sentinel(#<process notmuch-search> "finished\n")

This happens on the current version of OpenIndiana (a fork of a fork of
a fork of a fork of Solaris) as well when running:

(emacs-version)
"GNU Emacs 25.3.1 (i386-pc-solaris2.11, GTK+ Version 2.24.31)
 of 2017-09-12"

> A quick look at the implementation of `make-process' in the Carbon port
> didn't reveal anything obvious to me. This mostly seems like a race -
> whether emacs has decided that the process associated with the stderr
> buffer is dead or not when we call `kill-buffer'. Is any ordering
> guaranteed by the implementation?
>
> I _think_ that have also seen the same problem when asynchronous address
> harvesting is happening for completion on the default NextStep port for
> macOS, but haven't been able to reliably reproduce it.
>
> dme.
> --
> I got a girlfriend that's better than that.

dme.
--
So think of Bob and Judy, they're happy as can be.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

[David Edmondson]

On some platforms (e.g. macOS), it is necessary to add a real sentinel
process for the error buffer used by `notmuch-start-notmuch' rather
than a no-op sentinel.
---
 emacs/notmuch-lib.el | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index a7e02710..03c966b7 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -909,7 +909,7 @@ invoke `set-process-sentinel' directly on the returned process,
 as that will interfere with the handling of stderr and the exit
 status."
 
-  (let (err-file err-buffer proc
+  (let (err-file err-buffer proc err-proc
  ;; Find notmuch using Emacs' `exec-path'
  (command (or (executable-find notmuch-command)
      (error "Command not found: %s" notmuch-command))))
@@ -926,11 +926,13 @@ status."
       :buffer buffer
       :command (cons command args)
       :connection-type 'pipe
-      :stderr err-buffer))
+      :stderr err-buffer)
+ err-proc (get-buffer-process err-buffer))
   (process-put proc 'err-buffer err-buffer)
-  ;; Silence "Process NAME stderr finished" in stderr by adding a
-  ;; no-op sentinel to the fake stderr process object
-  (set-process-sentinel (get-buffer-process err-buffer) #'ignore))
+
+  (process-put err-proc 'err-file err-file)
+  (process-put err-proc 'err-buffer err-buffer)
+  (set-process-sentinel err-proc #'notmuch-start-notmuch-error-sentinel))
 
       ;; On Emacs versions before 25, there is no way to capture
       ;; stdout and stderr separately for asynchronous processes, or
@@ -990,9 +992,14 @@ status."
        ;; Emacs behaves strangely if an error escapes from a sentinel,
        ;; so turn errors into messages.
        (message "%s" (error-message-string err))))
-    (when err-buffer (kill-buffer err-buffer))
     (when err-file (ignore-errors (delete-file err-file)))))
 
+(defun notmuch-start-notmuch-error-sentinel (proc event)
+  (let* ((err-file (process-get proc 'err-file))
+ (err-buffer (or (process-get proc 'err-buffer)
+ (find-file-noselect err-file))))
+    (when err-buffer (kill-buffer err-buffer))))
+
 ;; This variable is used only buffer local, but it needs to be
 ;; declared globally first to avoid compiler warnings.
 (defvar notmuch-show-process-crypto nil)
--
2.17.1 (Apple Git-112)

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

[David Edmondson]

This failure mode is very annoying. Could I persuade someone to review
the proposed change?

On Thursday, 2018-08-09 at 21:54:34 +01, David Edmondson wrote:

> On some platforms (e.g. macOS), it is necessary to add a real sentinel
> process for the error buffer used by `notmuch-start-notmuch' rather
> than a no-op sentinel.
> ---
>  emacs/notmuch-lib.el | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index a7e02710..03c966b7 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -909,7 +909,7 @@ invoke `set-process-sentinel' directly on the returned process,
>  as that will interfere with the handling of stderr and the exit
>  status."
>  
> -  (let (err-file err-buffer proc
> +  (let (err-file err-buffer proc err-proc
>   ;; Find notmuch using Emacs' `exec-path'
>   (command (or (executable-find notmuch-command)
>       (error "Command not found: %s" notmuch-command))))
> @@ -926,11 +926,13 @@ status."
>        :buffer buffer
>        :command (cons command args)
>        :connection-type 'pipe
> -      :stderr err-buffer))
> +      :stderr err-buffer)
> + err-proc (get-buffer-process err-buffer))
>    (process-put proc 'err-buffer err-buffer)
> -  ;; Silence "Process NAME stderr finished" in stderr by adding a
> -  ;; no-op sentinel to the fake stderr process object
> -  (set-process-sentinel (get-buffer-process err-buffer) #'ignore))
> +
> +  (process-put err-proc 'err-file err-file)
> +  (process-put err-proc 'err-buffer err-buffer)
> +  (set-process-sentinel err-proc #'notmuch-start-notmuch-error-sentinel))
>  
>        ;; On Emacs versions before 25, there is no way to capture
>        ;; stdout and stderr separately for asynchronous processes, or
> @@ -990,9 +992,14 @@ status."
>         ;; Emacs behaves strangely if an error escapes from a sentinel,
>         ;; so turn errors into messages.
>         (message "%s" (error-message-string err))))
> -    (when err-buffer (kill-buffer err-buffer))
>      (when err-file (ignore-errors (delete-file err-file)))))
>  
> +(defun notmuch-start-notmuch-error-sentinel (proc event)
> +  (let* ((err-file (process-get proc 'err-file))
> + (err-buffer (or (process-get proc 'err-buffer)
> + (find-file-noselect err-file))))
> +    (when err-buffer (kill-buffer err-buffer))))
> +
>  ;; This variable is used only buffer local, but it needs to be
>  ;; declared globally first to avoid compiler warnings.
>  (defvar notmuch-show-process-crypto nil)
> --
> 2.17.1 (Apple Git-112)

dme.
--
In heaven there is no beer, that's why we drink it here.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

[Sebastian Schwarz]

On 2017-10-26, David Edmondson wrote:
> After the changes to use `make-process', the Carbon port of emacs on
> macOS (often referred to as emacs-mac or the railwaycat port) will ask
> about killing the stderr buffer after any `notmuch-search':
>
> Debugger entered--Lisp error: (quit)
>   yes-or-no-p("Buffer \" *notmuch-stderr*-839121\" has a running process; kill it? ")
>   process-kill-buffer-query-function()
>   kill-buffer(#<buffer  *notmuch-stderr*-839121>)
>   notmuch-start-notmuch-sentinel(#<process notmuch-search> "finished\n")

I see the same prompt whenever I call the function
notmuch-refresh-all-buffers on Ubuntu 18.04, which has GNU Emacs
25.2 and notmuch 0.26.

On 2018-08-09, David Edmondson wrote:
> On some platforms (e.g. macOS), it is necessary to add a real sentinel
> process for the error buffer used by `notmuch-start-notmuch' rather
> than a no-op sentinel.

Applying your patch fixes this for me there.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

[David Bremner-2]

David Edmondson <[hidden email]> writes:
>  
> +(defun notmuch-start-notmuch-error-sentinel (proc event)
> +  (let* ((err-file (process-get proc 'err-file))
> + (err-buffer (or (process-get proc 'err-buffer)
> + (find-file-noselect err-file))))
Is the second case here (find-file-noselect) for the non-make-process
code path, or something else? It might help to have a comment.
> +    (when err-buffer (kill-buffer err-buffer))))
> +

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

[David Edmondson]

On Sunday, 2018-08-26 at 08:30:41 -03, David Bremner wrote:

> David Edmondson <[hidden email]> writes:
>>  
>> +(defun notmuch-start-notmuch-error-sentinel (proc event)
>> +  (let* ((err-file (process-get proc 'err-file))
>> + (err-buffer (or (process-get proc 'err-buffer)
>> + (find-file-noselect err-file))))
> Is the second case here (find-file-noselect) for the non-make-process
> code path, or something else? It might help to have a comment.

For non-make-process.  Updated patch sent.

>> +    (when err-buffer (kill-buffer err-buffer))))
>> +

dme.
--
I'm not the reason you're looking for redemption.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch