* [PATCH 0/2] Fix errors when killing the search buffer @ 2013-06-09 4:45 Austin Clements 2013-06-09 4:45 ` [PATCH 1/2] emacs: Don't report CLI signals sent by Emacs as errors Austin Clements 2013-06-09 4:45 ` [PATCH 2/2] emacs: Fix "no such file or directory" error Austin Clements 0 siblings, 2 replies; 7+ messages in thread From: Austin Clements @ 2013-06-09 4:45 UTC (permalink / raw) To: notmuch This series fixes the first two problems described by Jameson in id:87r4gk8qa5.fsf@servo.finestructure.net that caused errors to be reported when the search buffer was killed by the user while the search was still running. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] emacs: Don't report CLI signals sent by Emacs as errors 2013-06-09 4:45 [PATCH 0/2] Fix errors when killing the search buffer Austin Clements @ 2013-06-09 4:45 ` Austin Clements 2013-06-12 14:57 ` David Bremner 2013-06-09 4:45 ` [PATCH 2/2] emacs: Fix "no such file or directory" error Austin Clements 1 sibling, 1 reply; 7+ messages in thread From: Austin Clements @ 2013-06-09 4:45 UTC (permalink / raw) To: notmuch Previously, when the user killed the search buffer before the CLI search process had completed, we would report the signal sent by Emacs to kill the CLI to the user as an error. Fix this by only reporting error exits if the process buffer is still live. We still report stderr output regardless in case stderr output was relevant to why the user killed the search buffer (such as a wrapper script being stuck). --- emacs/notmuch-lib.el | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el index 28f78e0..534f217 100644 --- a/emacs/notmuch-lib.el +++ b/emacs/notmuch-lib.el @@ -528,8 +528,12 @@ status." (when sub-sentinel (funcall sub-sentinel proc event)) ;; Check the exit status. This will signal an error if the - ;; exit status is non-zero. - (notmuch-check-async-exit-status proc event real-command err-file) + ;; exit status is non-zero. Don't do this if the process + ;; buffer is dead since that means Emacs killed the process + ;; and there's no point in telling the user that (but we + ;; still check for and report stderr output below). + (when (buffer-live-p (process-buffer proc)) + (notmuch-check-async-exit-status proc event real-command err-file)) ;; If that didn't signal an error, then any error output was ;; really warning output. Show warnings, if any. (let ((warnings -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] emacs: Don't report CLI signals sent by Emacs as errors 2013-06-09 4:45 ` [PATCH 1/2] emacs: Don't report CLI signals sent by Emacs as errors Austin Clements @ 2013-06-12 14:57 ` David Bremner 0 siblings, 0 replies; 7+ messages in thread From: David Bremner @ 2013-06-12 14:57 UTC (permalink / raw) To: Austin Clements, notmuch Austin Clements <amdragon@MIT.EDU> writes: > Previously, when the user killed the search buffer before the CLI > search process had completed, we would report the signal sent by Emacs > to kill the CLI to the user as an error. pushed both. d ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] emacs: Fix "no such file or directory" error 2013-06-09 4:45 [PATCH 0/2] Fix errors when killing the search buffer Austin Clements 2013-06-09 4:45 ` [PATCH 1/2] emacs: Don't report CLI signals sent by Emacs as errors Austin Clements @ 2013-06-09 4:45 ` Austin Clements 2013-06-09 9:16 ` Mark Walters 1 sibling, 1 reply; 7+ messages in thread From: Austin Clements @ 2013-06-09 4:45 UTC (permalink / raw) To: notmuch Occasionally, when the user killed the search buffer when the CLI process was still running, Emacs would run the notmuch-start-notmuch-sentinel sentinel twice. The first call would process and delete the error output file and the second would fail with an "Opening input file: no such file or directory, ..." error when attempting to access the error file. Emacs isn't supposed to run the sentinel twice. The reason it does is rather subtle (and probably a bug in Emacs): 1) When the user kills the search buffer, Emacs invokes kill_buffer_processes, which sends a SIGHUP to notmuch, but doesn't do anything else. Meanwhile, suppose the notmuch search process has printed some more output, but Emacs hasn't consumed it yet (this is critical and is why this error only happens sometimes). 2) Emacs gets a SIGCHLD from the dying notmuch process, which invokes handle_child_signal, which sets the new process status, but can't do anything else because it's a signal handler. 3) Emacs returns to its idle loop, which calls status_notify, which sees that the notmuch process has a new status. This is where things get interesting. 3.1) Emacs guarantees that it will run process filters on any unconsumed output before running the process sentinel, so status_notify calls read_process_output, which consumes the final output and calls notmuch-search-process-filter. 3.1.1) notmuch-search-process-filter checks if the search buffer is still alive and, since it's not, it calls delete-process. 3.1.1.1) delete-process correctly sees that the process is already dead and doesn't try to send another signal, *but* it still modifies the status to "killed". To deal with the new status, it calls status_notify. Dun dun dun. We've seen this function before. 3.1.1.1.1) The *recursive* status_notify invocation sees that the process has a new status and doesn't have any more output to consume, so it invokes our sentinel and returns. 3.2) The outer status_notify call (which we're still in) is now done flushing pending process output, so it *also* invokes our sentinel. This patch addresses this problem at step 3.1.1, where the filter calls delete-process, since this is a strange and redundant thing to do anyway. --- emacs/notmuch.el | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/emacs/notmuch.el b/emacs/notmuch.el index 7994d74..a9949a1 100644 --- a/emacs/notmuch.el +++ b/emacs/notmuch.el @@ -821,8 +821,7 @@ non-authors is found, assume that all of the authors match." (parse-buf (process-get proc 'parse-buf)) (inhibit-read-only t) done) - (if (not (buffer-live-p results-buf)) - (delete-process proc) + (when (buffer-live-p results-buf) (with-current-buffer parse-buf ;; Insert new data (save-excursion -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] emacs: Fix "no such file or directory" error 2013-06-09 4:45 ` [PATCH 2/2] emacs: Fix "no such file or directory" error Austin Clements @ 2013-06-09 9:16 ` Mark Walters 2013-06-10 2:15 ` Austin Clements 0 siblings, 1 reply; 7+ messages in thread From: Mark Walters @ 2013-06-09 9:16 UTC (permalink / raw) To: Austin Clements, notmuch Both of these patches look good to me +1. I was able to reproduce both bugs pretty reliably (the first one always unless masked by the second one which occurred about half the time). With these patches I cannot trigger either. Also all tests pass. My only tiny concern is I couldn't find any documentation on whether the return value of the filter-function matters at all. Austin's original fix (via irc) returned t and this returns nil in the failing case (i.e., when results-buf is dead). Best wishes Mark On Sun, 09 Jun 2013, Austin Clements <amdragon@MIT.EDU> wrote: > Occasionally, when the user killed the search buffer when the CLI > process was still running, Emacs would run the > notmuch-start-notmuch-sentinel sentinel twice. The first call would > process and delete the error output file and the second would fail > with an "Opening input file: no such file or directory, ..." error > when attempting to access the error file. > > Emacs isn't supposed to run the sentinel twice. The reason it does is > rather subtle (and probably a bug in Emacs): > > 1) When the user kills the search buffer, Emacs invokes > kill_buffer_processes, which sends a SIGHUP to notmuch, but doesn't do > anything else. Meanwhile, suppose the notmuch search process has > printed some more output, but Emacs hasn't consumed it yet (this is > critical and is why this error only happens sometimes). > > 2) Emacs gets a SIGCHLD from the dying notmuch process, which invokes > handle_child_signal, which sets the new process status, but can't do > anything else because it's a signal handler. > > 3) Emacs returns to its idle loop, which calls status_notify, which > sees that the notmuch process has a new status. This is where things > get interesting. > > 3.1) Emacs guarantees that it will run process filters on any > unconsumed output before running the process sentinel, so > status_notify calls read_process_output, which consumes the final > output and calls notmuch-search-process-filter. > > 3.1.1) notmuch-search-process-filter checks if the search buffer is > still alive and, since it's not, it calls delete-process. > > 3.1.1.1) delete-process correctly sees that the process is already > dead and doesn't try to send another signal, *but* it still modifies > the status to "killed". To deal with the new status, it calls > status_notify. Dun dun dun. We've seen this function before. > > 3.1.1.1.1) The *recursive* status_notify invocation sees that the > process has a new status and doesn't have any more output to consume, > so it invokes our sentinel and returns. > > 3.2) The outer status_notify call (which we're still in) is now done > flushing pending process output, so it *also* invokes our sentinel. > > This patch addresses this problem at step 3.1.1, where the filter > calls delete-process, since this is a strange and redundant thing to > do anyway. > --- > emacs/notmuch.el | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/emacs/notmuch.el b/emacs/notmuch.el > index 7994d74..a9949a1 100644 > --- a/emacs/notmuch.el > +++ b/emacs/notmuch.el > @@ -821,8 +821,7 @@ non-authors is found, assume that all of the authors match." > (parse-buf (process-get proc 'parse-buf)) > (inhibit-read-only t) > done) > - (if (not (buffer-live-p results-buf)) > - (delete-process proc) > + (when (buffer-live-p results-buf) > (with-current-buffer parse-buf > ;; Insert new data > (save-excursion > -- > 1.7.10.4 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] emacs: Fix "no such file or directory" error 2013-06-09 9:16 ` Mark Walters @ 2013-06-10 2:15 ` Austin Clements 2013-06-10 6:21 ` Tomi Ollila 0 siblings, 1 reply; 7+ messages in thread From: Austin Clements @ 2013-06-10 2:15 UTC (permalink / raw) To: Mark Walters; +Cc: notmuch Quoth Mark Walters on Jun 09 at 10:16 am: > > Both of these patches look good to me +1. I was able to reproduce both > bugs pretty reliably (the first one always unless masked by the second > one which occurred about half the time). With these patches I cannot > trigger either. Also all tests pass. > > My only tiny concern is I couldn't find any documentation on whether the > return value of the filter-function matters at all. Austin's original > fix (via irc) returned t and this returns nil in the failing case (i.e., > when results-buf is dead). Mm, interesting. To be fair, my choice of "t" for the original fix was completely arbitrary. I think you're right that the Emacs documentation doesn't have anything to say about the return values of filter functions. Furthermore, the example filter functions they give don't have meaningful return values, so I'm pretty sure this is safe. Also the code that calls the filter discards its result. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] emacs: Fix "no such file or directory" error 2013-06-10 2:15 ` Austin Clements @ 2013-06-10 6:21 ` Tomi Ollila 0 siblings, 0 replies; 7+ messages in thread From: Tomi Ollila @ 2013-06-10 6:21 UTC (permalink / raw) To: Austin Clements, Mark Walters; +Cc: notmuch On Mon, Jun 10 2013, Austin Clements <amdragon@MIT.EDU> wrote: > Quoth Mark Walters on Jun 09 at 10:16 am: >> >> Both of these patches look good to me +1. I was able to reproduce both >> bugs pretty reliably (the first one always unless masked by the second >> one which occurred about half the time). With these patches I cannot >> trigger either. Also all tests pass. >> >> My only tiny concern is I couldn't find any documentation on whether the >> return value of the filter-function matters at all. Austin's original >> fix (via irc) returned t and this returns nil in the failing case (i.e., >> when results-buf is dead). > > Mm, interesting. To be fair, my choice of "t" for the original fix > was completely arbitrary. I think you're right that the Emacs > documentation doesn't have anything to say about the return values of > filter functions. Furthermore, the example filter functions they give > don't have meaningful return values, so I'm pretty sure this is safe. > Also the code that calls the filter discards its result. I also looked this a bit yesterday evening. For example this page http://www.gnu.org/software/emacs/manual/html_node/elisp/Filter-Functions.html discusses only about catching thrown errors -- i.e. no mention about filter function return values. From that I'd draw a conclusion that most probably the return value of filter function is not used for anything. ... and the patch looks good. +1 (removing needs-review) Tomi ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-06-12 14:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-09 4:45 [PATCH 0/2] Fix errors when killing the search buffer Austin Clements 2013-06-09 4:45 ` [PATCH 1/2] emacs: Don't report CLI signals sent by Emacs as errors Austin Clements 2013-06-12 14:57 ` David Bremner 2013-06-09 4:45 ` [PATCH 2/2] emacs: Fix "no such file or directory" error Austin Clements 2013-06-09 9:16 ` Mark Walters 2013-06-10 2:15 ` Austin Clements 2013-06-10 6:21 ` Tomi Ollila
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).