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 at 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 at notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch