* Race condition for '*' command @ 2011-06-25 22:18 Robin Green 2011-06-25 23:57 ` Jameson Graef Rollins 0 siblings, 1 reply; 10+ messages in thread From: Robin Green @ 2011-06-25 22:18 UTC (permalink / raw) To: notmuch Hi, A race condition in the '*' command was noted when it was first proposed. It looks to me like it still exists - has anything been done about it? Right now what I am doing is killing fetchmail, then using the '*' command, then starting fetchmail again in daemon mode, which is a little inconvenient. You see, in my setup fetchmail calls procmail to deliver mails, and procmail calls "notmuch new" after delivering every mail. Thanks, Robin Green ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race condition for '*' command 2011-06-25 22:18 Race condition for '*' command Robin Green @ 2011-06-25 23:57 ` Jameson Graef Rollins 2011-06-26 9:00 ` Robin Green 0 siblings, 1 reply; 10+ messages in thread From: Jameson Graef Rollins @ 2011-06-25 23:57 UTC (permalink / raw) To: Robin Green, notmuch [-- Attachment #1: Type: text/plain, Size: 507 bytes --] On Sat, 25 Jun 2011 23:18:52 +0100, Robin Green <greenrd@greenrd.org> wrote: > A race condition in the '*' command was noted when it was first > proposed. It looks to me like it still exists - has anything been done > about it? Hi, Robin. Can you explain what you mean by the "'*' command"? Are you referring to '*' as a search term? It's not a command, but it's the only place I know of where notmuch uses the '*' character. Can you explain in a little more detail what exactly your issue is? jamie. [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race condition for '*' command 2011-06-25 23:57 ` Jameson Graef Rollins @ 2011-06-26 9:00 ` Robin Green 2011-06-28 6:49 ` Pieter Praet 0 siblings, 1 reply; 10+ messages in thread From: Robin Green @ 2011-06-26 9:00 UTC (permalink / raw) To: notmuch On Sat, 25 Jun 2011 16:57:50 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote: > On Sat, 25 Jun 2011 23:18:52 +0100, Robin Green <greenrd@greenrd.org> wrote: > > A race condition in the '*' command was noted when it was first > > proposed. It looks to me like it still exists - has anything been done > > about it? > > Hi, Robin. Can you explain what you mean by the "'*' command"? Sorry - forgot to say I'm talking about the notmuch emacs mode. In that mode '*' applies tags to all messages matching the current search query, which means that (here's the race condition) new results that have appeared since the last refresh will also be tagged. -- Robin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race condition for '*' command 2011-06-26 9:00 ` Robin Green @ 2011-06-28 6:49 ` Pieter Praet 2011-06-28 17:36 ` Mark Anderson 0 siblings, 1 reply; 10+ messages in thread From: Pieter Praet @ 2011-06-28 6:49 UTC (permalink / raw) To: Robin Green, notmuch On Sun, 26 Jun 2011 10:00:41 +0100, Robin Green <greenrd@greenrd.org> wrote: > On Sat, 25 Jun 2011 16:57:50 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote: > > On Sat, 25 Jun 2011 23:18:52 +0100, Robin Green <greenrd@greenrd.org> wrote: > > > A race condition in the '*' command was noted when it was first > > > proposed. It looks to me like it still exists - has anything been done > > > about it? > > > > Hi, Robin. Can you explain what you mean by the "'*' command"? > > Sorry - forgot to say I'm talking about the notmuch emacs mode. In that > mode '*' applies tags to all messages matching the current search query, > which means that (here's the race condition) new results that have > appeared since the last refresh will also be tagged. This issue appears to stem from the fact that `notmuch-search-operate-all' runs (apply 'notmuch-tag notmuch-search-query-string action-split), in which `notmuch-search-query-string' points to a moving target. Could be solved by doing it with `notmuch-search', `mark-whole-buffer' and `notmuch-search-{add,remove}-tag-region' instead, but I'm sure there's a better way (of which I'm as of yet unaware). > -- > Robin > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch Peace -- Pieter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race condition for '*' command 2011-06-28 6:49 ` Pieter Praet @ 2011-06-28 17:36 ` Mark Anderson 2011-06-28 19:48 ` Carl Worth 0 siblings, 1 reply; 10+ messages in thread From: Mark Anderson @ 2011-06-28 17:36 UTC (permalink / raw) To: Pieter Praet, Robin Green, notmuch On Tue, 28 Jun 2011 08:49:06 +0200, Pieter Praet <pieter@praet.org> wrote: > On Sun, 26 Jun 2011 10:00:41 +0100, Robin Green <greenrd@greenrd.org> wrote: > > On Sat, 25 Jun 2011 16:57:50 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote: > > > On Sat, 25 Jun 2011 23:18:52 +0100, Robin Green <greenrd@greenrd.org> wrote: > > > > A race condition in the '*' command was noted when it was first > > > > proposed. It looks to me like it still exists - has anything been done > > > > about it? > > > > > > Hi, Robin. Can you explain what you mean by the "'*' command"? > > > > Sorry - forgot to say I'm talking about the notmuch emacs mode. In that > > mode '*' applies tags to all messages matching the current search query, > > which means that (here's the race condition) new results that have > > appeared since the last refresh will also be tagged. > > This issue appears to stem from the fact that `notmuch-search-operate-all' > runs (apply 'notmuch-tag notmuch-search-query-string action-split), in which > `notmuch-search-query-string' points to a moving target. > > Could be solved by doing it with `notmuch-search', `mark-whole-buffer' > and `notmuch-search-{add,remove}-tag-region' instead, but I'm sure > there's a better way (of which I'm as of yet unaware). I don't think there's a better way, the results which the user is viewing is the only visual reference for what the action they intend to apply will use as the object of said action. I can imagine some people wanting to have a way to apply an action to a live query string, but I distinguish between these people and normal humans reading their email. I expect that when a normal human is reading their email, they do not intend to apply actions to messages as yet unseen. I can see some people using the '*' notmuch emacs command as a very interactive notmuch-poll.sh script. But I imagine the principle of least surprise would be to have '*' only apply to visible messages, and if the buffer is old, the buffer is the only record of what is being viewed. -Mark > > > -- > > Robin > > _______________________________________________ > > notmuch mailing list > > notmuch@notmuchmail.org > > http://notmuchmail.org/mailman/listinfo/notmuch > > Peace > > -- > Pieter > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race condition for '*' command 2011-06-28 17:36 ` Mark Anderson @ 2011-06-28 19:48 ` Carl Worth 2011-06-30 16:08 ` [PATCH 1/2] test: add/remove tags from all matching messages with `notmuch-search-operate-all' Pieter Praet 0 siblings, 1 reply; 10+ messages in thread From: Carl Worth @ 2011-06-28 19:48 UTC (permalink / raw) To: Mark Anderson, Pieter Praet, Robin Green, notmuch [-- Attachment #1: Type: text/plain, Size: 3162 bytes --] On Tue, 28 Jun 2011 11:36:10 -0600, Mark Anderson <ma.skies@gmail.com> wrote: > On Tue, 28 Jun 2011 08:49:06 +0200, Pieter Praet <pieter@praet.org> wrote: > > On Sun, 26 Jun 2011 10:00:41 +0100, Robin Green <greenrd@greenrd.org> wrote: > > > On Sat, 25 Jun 2011 16:57:50 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote: > > > > On Sat, 25 Jun 2011 23:18:52 +0100, Robin Green <greenrd@greenrd.org> wrote: > > > > > A race condition in the '*' command was noted when it was first > > > > > proposed. It looks to me like it still exists - has anything been done > > > > > about it? Not much. I did add this note to notmuch/TODO at the time: Fix '*' to work by simply calling '+' or '-' on a region consisting of the entire buffer > > Could be solved by doing it with `notmuch-search', `mark-whole-buffer' > > and `notmuch-search-{add,remove}-tag-region' instead, but I'm sure > > there's a better way (of which I'm as of yet unaware). That's a more concrete description of the same idea. I did some experiments at one point and determined that doing this would make '*' far too painful to be usable with large search-result buffers, (which is precisely when I am most likely to use it). But even if we were willing to accept the performance penalty, (or if we fixed notmuch-search-*-tag-region to accumulate the thread IDs and make a single invocation of notmuch to avoid the performance penalty). Even then, there would still be race conditions here, (though more subtle than the current race). At that point we would only be acting on threads that matched the original search, but we could be acting on more messages than were originally presented to the user. For example, a thread might have been displayed as having only one post, but the "* -inbox" operation would remove the inbox tag from any replies received in the interim. I'd love to find some clean solutions here, as I would love notmuch to offer an interface that is free of these race conditions, (which are scary since they can prevent a user from seeing some mail). Personally, I'm avoiding all races right now by only running "notmuch new" manually, (and not otherwise interacting with notmuch while "notmuch new" is running). This does avoid the races, but is fairly awkward, (I have to manually run this (or use the annoyingly blocking 'G'[*] binding), so I would prefer the ability to have new mail incorporated automatically as it arrives. Independent of this particular race condition, having something like fetchmail automatically invoke "notmuch new" also has the potential to break your mail client with Xapian Database-modified exceptions. So we do need some more direct support for this mode of operation. In the meantime, I've at least updated the TODO note a bit: Fix '*' to work by simply calling '+' or '-' on a region consisting of the entire buffer, (this would avoid one race condition---while still leaving other race conditions---but could also potentially make '*' a very expensive operation). -Carl [*] notmuch-search-poll-and-refresh-view -- carl.d.worth@intel.com [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] test: add/remove tags from all matching messages with `notmuch-search-operate-all' 2011-06-28 19:48 ` Carl Worth @ 2011-06-30 16:08 ` Pieter Praet 2011-06-30 16:08 ` [PATCH 2/2] [RFC] possible solution for "Race condition for '*' command" Pieter Praet 0 siblings, 1 reply; 10+ messages in thread From: Pieter Praet @ 2011-06-30 16:08 UTC (permalink / raw) To: Carl Worth, Mark Anderson, Robin Green; +Cc: notmuch This test is meant solely to make sure my next patch doesn't break anything (in an obvious way, at least). In other words: it does *not* demonstrate a race condition issue. Signed-off-by: Pieter Praet <pieter@praet.org> --- Robin, Could you provide a test to expose the actual race condition? Or at the very least something to mitigate my severe clue-deficiency? Peace test/emacs | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) diff --git a/test/emacs b/test/emacs index 53f455a..ffe8a55 100755 --- a/test/emacs +++ b/test/emacs @@ -347,4 +347,23 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail. (test-visible-output)' test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-thread-with-hidden-messages +test_begin_subtest "Add/remove tags from all matching messages." +test_emacs '(notmuch-search "tag:inbox AND tags") + (notmuch-test-wait) + (notmuch-search-operate-all "+matching -inbox") + (notmuch-search "tag:matching") + (notmuch-test-wait) + (test-output)' +cat <<EOF >EXPECTED + 2009-11-18 [3/3] Adrian Perez de Castro, Keith Packard, Carl Worth [notmuch] Introducing myself (matching signed unread) + 2009-11-18 [1/3] Carl Worth, Israel Herraiz, Keith Packard [notmuch] New to the list (inbox matching unread) + 2009-11-18 [2/2] Keith Packard, Carl Worth [notmuch] [PATCH] Make notmuch-show 'X' (and 'x') commands remove inbox (and unread) tags (matching unread) + 2009-11-18 [1/2] Keith Packard, Alexander Botero-Lowry [notmuch] [PATCH] Create a default notmuch-show-hook that highlights URLs and uses word-wrap (inbox matching unread) + 2009-11-18 [1/1] Jan Janak [notmuch] [PATCH] notmuch new: Support for conversion of spool subdirectories into tags (matching unread) + 2009-11-18 [1/1] Stewart Smith [notmuch] [PATCH] Fix linking with gcc to use g++ to link in C++ libs. (matching unread) + 2009-11-17 [1/2] Ingmar Vanhassel, Carl Worth [notmuch] [PATCH] Typsos (inbox matching unread) +End of search results. +EOF +test_expect_equal_file OUTPUT EXPECTED + test_done -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] [RFC] possible solution for "Race condition for '*' command" 2011-06-30 16:08 ` [PATCH 1/2] test: add/remove tags from all matching messages with `notmuch-search-operate-all' Pieter Praet @ 2011-06-30 16:08 ` Pieter Praet 2011-06-30 19:38 ` Pieter Praet 0 siblings, 1 reply; 10+ messages in thread From: Pieter Praet @ 2011-06-30 16:08 UTC (permalink / raw) To: Carl Worth, Mark Anderson, Robin Green; +Cc: notmuch `notmuch-search-operate-all' may cause a race condition because repeatedly running `notmuch-tag' with the *original* query string makes the result list a moving target. One approach to resolving this, is to feed `notmuch-tag' a static result list based on the original query string, instead of the latter itself. See discussion @ id:"86d3i1d06r.fsf@dragonfly.greenrd.org" Signed-off-by: Pieter Praet <pieter@praet.org> --- Carl, I've gone along a different route which assures only matched messages are touched, but it does come with quite a performance hit. Since there isn't a test for the actual race condition(s), I can't be sure, but theoretically, at least one of them should be fixed now. Peace emacs/notmuch.el | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/emacs/notmuch.el b/emacs/notmuch.el index f11ec24..84c3ee6 100644 --- a/emacs/notmuch.el +++ b/emacs/notmuch.el @@ -845,7 +845,8 @@ Each character of the tag name may consist of alphanumeric characters as well as `_.+-'. " (interactive "sOperation (+add -drop): notmuch tag ") - (let ((action-split (split-string action " +"))) + (let ((action-split (split-string action " +")) + (query notmuch-search-query-string)) ;; Perform some validation (let ((words action-split)) (when (null words) (error "No operation given")) @@ -853,7 +854,13 @@ characters as well as `_.+-'. (unless (string-match-p "^[-+][-+_.[:word:]]+$" (car words)) (error "Action must be of the form `+thistag -that_tag'")) (setq words (cdr words)))) - (apply 'notmuch-tag notmuch-search-query-string action-split))) + (dolist (msgid + (split-string + (with-output-to-string + (with-current-buffer standard-output + (apply 'call-process notmuch-command nil t nil "search" "--output=messages" (list query)))) + "\n+" t)) + (apply 'notmuch-tag msgid action-split)))) (defun notmuch-search-buffer-title (query) "Returns the title for a buffer with notmuch search results." -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] [RFC] possible solution for "Race condition for '*' command" 2011-06-30 16:08 ` [PATCH 2/2] [RFC] possible solution for "Race condition for '*' command" Pieter Praet @ 2011-06-30 19:38 ` Pieter Praet 2011-07-01 14:55 ` Austin Clements 0 siblings, 1 reply; 10+ messages in thread From: Pieter Praet @ 2011-06-30 19:38 UTC (permalink / raw) To: Carl Worth, Mark Anderson, Robin Green; +Cc: notmuch On Thu, 30 Jun 2011 18:08:28 +0200, Pieter Praet <pieter@praet.org> wrote: > `notmuch-search-operate-all' may cause a race condition because > repeatedly running `notmuch-tag' with the *original* query string > makes the result list a moving target. > > One approach to resolving this, is to feed `notmuch-tag' a static result > list based on the original query string, instead of the latter itself. > > See discussion @ id:"86d3i1d06r.fsf@dragonfly.greenrd.org" > > Signed-off-by: Pieter Praet <pieter@praet.org> > --- > > Carl, > > I've gone along a different route which assures only matched messages > are touched, but it does come with quite a performance hit. > > Since there isn't a test for the actual race condition(s), I can't be > sure, but theoretically, at least one of them should be fixed now. > > Peace > > emacs/notmuch.el | 11 +++++++++-- > 1 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/emacs/notmuch.el b/emacs/notmuch.el > index f11ec24..84c3ee6 100644 > --- a/emacs/notmuch.el > +++ b/emacs/notmuch.el > @@ -845,7 +845,8 @@ Each character of the tag name may consist of alphanumeric > characters as well as `_.+-'. > " > (interactive "sOperation (+add -drop): notmuch tag ") > - (let ((action-split (split-string action " +"))) > + (let ((action-split (split-string action " +")) > + (query notmuch-search-query-string)) > ;; Perform some validation > (let ((words action-split)) > (when (null words) (error "No operation given")) > @@ -853,7 +854,13 @@ characters as well as `_.+-'. > (unless (string-match-p "^[-+][-+_.[:word:]]+$" (car words)) > (error "Action must be of the form `+thistag -that_tag'")) > (setq words (cdr words)))) > - (apply 'notmuch-tag notmuch-search-query-string action-split))) > + (dolist (msgid > + (split-string > + (with-output-to-string > + (with-current-buffer standard-output > + (apply 'call-process notmuch-command nil t nil "search" "--output=messages" (list query)))) > + "\n+" t)) > + (apply 'notmuch-tag msgid action-split)))) > > (defun notmuch-search-buffer-title (query) > "Returns the title for a buffer with notmuch search results." > -- > 1.7.4.1 > Ok, even though my very first reply [1] may have created the impression that I understood the issue, I clearly didn't... The test [2] needs a more applicable commit message, and the subsequent patch [3] points more or less in the right direction, but the Message-Id list should be local to the *search buffer* rather than to the `notmuch-search-operate-all' function. `notmuch-search' could: - run "notmuch-command search" with the "--output=messages" option instead of a plain search, - maintain a buffer-local var with a list of returned Message-Id's, - ...and populate the buffer based on that list. As such we'd have -for each individual search buffer- a canonical list of Message-Id's (i.e. messages which actually *match* the query AND are currently visible in the search buffer), to be used by `notmuch-search-operate-all' et al. Peace -- Pieter [1] id:"87fwmuxxgd.fsf@praet.org" [2] id:"1309450108-2793-2-git-send-email-pieter@praet.org" [3] id:"1309450108-2793-1-git-send-email-pieter@praet.org" ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] [RFC] possible solution for "Race condition for '*' command" 2011-06-30 19:38 ` Pieter Praet @ 2011-07-01 14:55 ` Austin Clements 0 siblings, 0 replies; 10+ messages in thread From: Austin Clements @ 2011-07-01 14:55 UTC (permalink / raw) To: Pieter Praet; +Cc: notmuch On Thu, Jun 30, 2011 at 3:38 PM, Pieter Praet <pieter@praet.org> wrote: > Ok, even though my very first reply [1] may have created the impression > that I understood the issue, I clearly didn't... > > The test [2] needs a more applicable commit message, and the subsequent > patch [3] points more or less in the right direction, but the Message-Id > list should be local to the *search buffer* rather than to the > `notmuch-search-operate-all' function. > > `notmuch-search' could: > - run "notmuch-command search" with the "--output=messages" option > instead of a plain search, > - maintain a buffer-local var with a list of returned Message-Id's, > - ...and populate the buffer based on that list. > > As such we'd have -for each individual search buffer- a canonical list > of Message-Id's (i.e. messages which actually *match* the query AND are > currently visible in the search buffer), to be used by > `notmuch-search-operate-all' et al. > > > Peace > > -- > Pieter > > [1] id:"87fwmuxxgd.fsf@praet.org" > [2] id:"1309450108-2793-2-git-send-email-pieter@praet.org" > [3] id:"1309450108-2793-1-git-send-email-pieter@praet.org" Ideally, this wouldn't be per-buffer, but per *line*. This race equally affects adding and removing tags from individual results, since that's done using a thread: query, whose results could have changed since the original search. This almost certainly requires support from the notmuch core. The good news is that the library already provides this information, so there will be virtually no performance hit for outputting it. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-07-01 14:55 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-06-25 22:18 Race condition for '*' command Robin Green 2011-06-25 23:57 ` Jameson Graef Rollins 2011-06-26 9:00 ` Robin Green 2011-06-28 6:49 ` Pieter Praet 2011-06-28 17:36 ` Mark Anderson 2011-06-28 19:48 ` Carl Worth 2011-06-30 16:08 ` [PATCH 1/2] test: add/remove tags from all matching messages with `notmuch-search-operate-all' Pieter Praet 2011-06-30 16:08 ` [PATCH 2/2] [RFC] possible solution for "Race condition for '*' command" Pieter Praet 2011-06-30 19:38 ` Pieter Praet 2011-07-01 14:55 ` Austin Clements
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).