From: Jameson Graef Rollins <jrollins@finestructure.net>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v3 0/6] Make Emacs search use sexp format
In-Reply-To: <87bo7mtp79.fsf@awakening.csail.mit.edu>
References: <1370047208-12785-1-git-send-email-amdragon@mit.edu>
	<87sj12yqyu.fsf@maritornes.cs.unb.ca>
	<87r4gk8qa5.fsf@servo.finestructure.net>
	<87bo7mtp79.fsf@awakening.csail.mit.edu>
User-Agent: Notmuch/0.15.2+155~g7fa0560 (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-pc-linux-gnu)
Date: Wed, 05 Jun 2013 08:21:59 -0700
Message-ID: <87a9n460rs.fsf@servo.finestructure.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha256; protocol="application/pgp-signature"
Cc: tomi.ollila@iki.fi
Precedence: list

--=-=-=
Content-Type: text/plain

On Mon, Jun 03 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> * Killing a search buffer that is still in the process of being filled
>>   causes errors to be thrown.  I'm seeing both of the following
>>   intermittently:
>>
>> [Sun Jun  2 08:26:40 2013]
>> notmuch exited with status killed
>> command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
>> exit signal: killed
>>
>> [Sun Jun  2 08:32:26 2013]
>> notmuch exited with status hangup
>> command: notmuch search --format\=sexp --format-version\=1 --sort\=newest-first to\:jrollins
>> exit signal: hangup
>>
>>   This is somewhat understandable, as the notmuch binary exits with an
>>   error if it hasn't finished dumping the output, but given how common
>>   this particular scenario is I think we should try to avoid throwing
>>   errors in this circumstance.  I wonder if we shouldn't just modify the
>>   binary to not return non-zero if it was manually killed while
>>   processing the output, or at least special-case the particular error
>>   caused by manually killing the search.
>
> Your assessment is correct, of course.  The right place to fix this is
> in Emacs, not the CLI (the CLI *can't* do anything about this, since it
> gets killed by a signal).  Probably we should do something different in
> the sentinel if the search process's buffer is no longer live.  Clearly
> we should suppress the status error for the signal, but I think we still
> should report anything that appeared in err-file because it may be
> relevant to why the user killed the buffer (e.g., maybe a notmuch
> wrapper was blocked on something).

That seems like a reasonable approach to me, to suppress the error but
continue to report in *Notmuch errors* buffer.

>> * The next thing I'm seeing is this:
>>
>> Opening input file: no such file or directory, /home/jrollins/tmp/nmerr5390CAY
>>
>>   I'm not exactly sure what causes this error, but it looks to me like
>>   the temporary error file was removed before we were finished with it.
>
> This one's pretty awesome (and I think is a bug in Emacs).  At a high
> level, the sentinel is getting run twice and since the first call
> deletes the error file, the second call fails.  At a low level, what
> causes this is fascinating.
>
> 1) You kill the search buffer.  This invokes kill_buffer_processes,
>    which sends a SIGHUP to notmuch, but doesn't do anything else.
>    Meanwhile, the notmuch search process has printed some more output,
>    but Emacs hasn't consumed it yet (this is critical).
>
> 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 contains code to check 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.
>
> It might be that the answer is to just remove the delete-process call
> from the filter.  It seems completely redundant (and racy) with Emacs'
> automatic SIGHUP'ing.

Wow, awesome detective work.  As mentioned on IRC, this suggestion of
Austin's does seem to fix the problem:

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 5a8c957..975ef2b 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -817,7 +817,7 @@ non-authors is found, assume that all of the authors match."
        (inhibit-read-only t)
        done)
     (if (not (buffer-live-p results-buf))
-       (delete-process proc)
+       t
       (with-current-buffer parse-buf
        ;; Insert new data
        (save-excursion


I'm not sure if this is the ultimate solution, but it does cause the
missing tmp file errors to go away.

>> * Finally, something happened that caused *12,000* of the following lines
>>   to be sent to the *Notmuch errors* buffer:
>>
>> A Xapian exception occurred performing query: The revision being read has been discarded - you should call Xapian::Database::reopen() and retry the operation
>>
>>   Again, this was related to killing a search buffer that was still
>>   being filled. I'm pretty sure the database was not modified during
>>   this process.
>
> I have no insight on this one.  My best guess is that this has nothing
> to do with this change except that this change makes these warnings
> visible rather than burying them somewhere down in the search results
> buffer.

Yeah, I suspected as much as well.

jamie.

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBCAAGBQJRr1eYAAoJEO00zqvie6q8d4IP/RWe/4CnDqQSm9QQLSoUfNwm
tIuDzsvo4reBrNlPwQrB5+jKtVTcd7byDv/OJcTbyr44M3qy/2LUzQOexm1WBGj1
wyg/qaiAKac0KjY/3zaxGwkXe/CMgfKJ3/zcBOnUFk3TadRs+KiKdZM6aS2KgQj/
OjSTZoVd9/6MTDf3je5fwcl+J74I0rOYmntK37PuRcMrNmSOsFxMH8sAvp8KgkFF
jY/IJ3mq/sS7/Juc02HN5IWKlByU6t7W3IqoYIbLfwT/TlyFZgMxXBHiae3nFYyx
WnJaYzU0zz7bFem0eka1uwhEQuvECeYBZmo5FrHF20uzPNpeMf2SH7P3hA39TQhn
E5HiKQzBNj5N67+7t8xrnaAbYPv+kTRO1iy7HjxjYoG0XcW9iJkz3lyqi9cqsLHu
gbA+VaiUBoEws3afDj3AJ+scDfW6pRgSWVi+nfMDaqziRKn3rKSquNjlXuWnW6AA
jjkL9P4OF3LpNJzilt1j+ocfqtmI86dj/Z8xK92Meh4zCBSMmq3DmXtkIm7uLQ8k
rOE4BCCNa1uuMBVP8x1iz3ogHg4Nygsjh8N8tgww+a50rWrXTv5FLAUDpwCTI7Fm
075Qgm7SiU06R4bF2YcOze/qJBg/sZyirm7ZfaYS7SiYZO1PP9vT4VIcxNviIExc
1WVjDwXoTkarE3EU6ika
=1AsS
-----END PGP SIGNATURE-----
--=-=-=--