From: Austin Clements Date: Tue, 4 Jun 2013 05:33:30 +0000 (+2000) Subject: Re: [PATCH v3 0/6] Make Emacs search use sexp format X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=98a30dd0dec650ea51db2911cad9f15ef7e68aa8;p=notmuch-archives.git Re: [PATCH v3 0/6] Make Emacs search use sexp format --- diff --git a/6d/e0ca24fe31bab03d8641e69c628077c6facdb5 b/6d/e0ca24fe31bab03d8641e69c628077c6facdb5 new file mode 100644 index 000000000..5dfb00255 --- /dev/null +++ b/6d/e0ca24fe31bab03d8641e69c628077c6facdb5 @@ -0,0 +1,196 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 1CFD4431FAF + for ; Mon, 3 Jun 2013 22:33:44 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id OJ1LkLe6edWA for ; + Mon, 3 Jun 2013 22:33:36 -0700 (PDT) +Received: from dmz-mailsec-scanner-4.mit.edu (dmz-mailsec-scanner-4.mit.edu + [18.9.25.15]) + by olra.theworths.org (Postfix) with ESMTP id D3775431FAE + for ; Mon, 3 Jun 2013 22:33:35 -0700 (PDT) +X-AuditID: 1209190f-b7fb76d0000008e6-e9-51ad7c2f95f3 +Received: from mailhub-auth-2.mit.edu ( [18.7.62.36]) + by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP + id F0.DE.02278.F2C7DA15; Tue, 4 Jun 2013 01:33:35 -0400 (EDT) +Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) + by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id r545XXtn026451; + Tue, 4 Jun 2013 01:33:33 -0400 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.8/8.12.4) with ESMTP id r545XU4w004212 + (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); + Tue, 4 Jun 2013 01:33:31 -0400 +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80) + (envelope-from ) + id 1UjjsA-0005St-F2; Tue, 04 Jun 2013 01:33:30 -0400 +From: Austin Clements +To: Jameson Graef Rollins , + notmuch@notmuchmail.org +Subject: Re: [PATCH v3 0/6] Make Emacs search use sexp format +In-Reply-To: <87r4gk8qa5.fsf@servo.finestructure.net> +References: <1370047208-12785-1-git-send-email-amdragon@mit.edu> + <87sj12yqyu.fsf@maritornes.cs.unb.ca> + <87r4gk8qa5.fsf@servo.finestructure.net> +User-Agent: Notmuch/0.15.2+173~g7cdb0c1 (http://notmuchmail.org) Emacs/23.4.1 + (i486-pc-linux-gnu) +Date: Tue, 04 Jun 2013 01:33:30 -0400 +Message-ID: <87bo7mtp79.fsf@awakening.csail.mit.edu> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFuplleLIzCtJLcpLzFFi42IRYrdT0dWvWRtocHWppMWN1m5Giz37vCyu + 35zJbPFm5TxWBxaPu6e5PA5/Xcji8WzVLWaPLYfeMwewRHHZpKTmZJalFunbJXBlzDp2k7Fg + v3bFs/6ljA2MG5W6GDk5JARMJG4/mMoOYYtJXLi3nq2LkYtDSGAfo8SUP5uYIZwNjBKXT6xh + hHBOMUnMePAWqmwJo8Sj28fYQPrZBDQktu1fzghiiwhESMy+up4ZxGYWsJT4c/8UWI2wgK3E + /9mvmEBsTgFTiSXbb0FN7WeU2LDqOFiRqECixN1l78FsFgFViTPPj7CA2LxAx1693sIIYQtK + nJz5hAVigZbEjX8vmSYwCs5CkpqFJLWAkWkVo2xKbpVubmJmTnFqsm5xcmJeXmqRrolebmaJ + XmpK6SZGUEhzSvLvYPx2UOkQowAHoxIPr8KWNYFCrIllxZW5hxglOZiURHm3lawNFOJLyk+p + zEgszogvKs1JLT7EKMHBrCTC2+sClONNSaysSi3Kh0lJc7AoifNeTbnpLySQnliSmp2aWpBa + BJOV4eBQkuDVrwZqFCxKTU+tSMvMKUFIM3FwggznARouAVLDW1yQmFucmQ6RP8Woy7H5/OR3 + jEIsefl5qVLivBkgRQIgRRmleXBzYKnoFaM40FvCvJEgVTzANAY36RXQEiagJZNfrwJZUpKI + kJJqYJQPKi2dtVc6+1z4zjy760J//z7LmyNQ16rau6Nkv2pgpMru8F+s4gu1pG0SQlmPGn/w + WijF8j5HZc9OxxMv9x1ZUqKizm11+Zp7CfOWy8YrmS6/LOUs7niy09w2VMT4tFVh2Lm/Ty02 + FVirfE1YefDUkfrwuMvuAQvkl2qs0LdY9m3JNke91UosxRmJhlrMRcWJAIRUfDIgAwAA +Cc: tomi.ollila@iki.fi +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Tue, 04 Jun 2013 05:33:44 -0000 + +On Sun, 02 Jun 2013, Jameson Graef Rollins wrote: +> On Sat, Jun 01 2013, David Bremner wrote: +>> Austin Clements writes: +>> +>>> This is v3 of id:1369934016-22308-1-git-send-email-amdragon@mit.edu. +>>> This tweaks the shell invocation as suggested by Tomi and fixes two +>>> comment typos pointed out by Mark. It also adds a NEWS patch. I'm +>>> going to go ahead and mark this ready because of Tomi's and Mark's +>>> reviews of v2. +>> +>> The first 5 I pushed. The NEWS patch has a conflict. +> +> I'm very happy to see the long-coming sexp handling working here. Good +> work, folks, particularly to Austin for getting the awesome asynchronous +> processing stuff working. Searches are now definitely noticeably +> faster. +> +> I am, however, seeing a couple of issues that we might want to address. +> +> * 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). + +> * 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. + +> * 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. + +> Let me know if I can help provide any more info. +> +> jamie.