Re: [mmh] show sequences racecondition
[2020-03-21 23:29] markus schnalke > [2020-03-21 16:15] Philipp Takacs > Thanks for the explanation. Show(1) is the only case where I've > noticed the problem. Thus it's the most important. That's the reason I have started there. I would say I look at the patches again, to see if I have overseen something and push them in the next days. > > I found then, that seq_read and seq_list ignores mails witch it thinks > > don't exist. This I have disabled this, to avoid a secound read of the > > of the whole directory. > > > > Now the conflict window is small, but still there. The best way to > > handle this is locking. > > This whole topic is a special one ... One case that one should > have in mind as well: Stuff shouldn't mess up, if you have show(1) > running while deleting/refiling/incing messages, not only to the > end but possibly on previously used message numbers. For instance, > I use `folder -pack' on the inbox quite often ... there might be a > `repl' job in background at the same time ... I have the problems with sortm and folder -pack in mind. I have some idea how to prevent most of such problems. But the problem is: We can help the user to prevent such problems, but there is no way we can stop the user from shooting him in the food. ;-) > I really don't want to burden you with this stuff. I think that > improving the most annoying problem is already a great step > forward. The rest is more of a separate topic to dig into and > solve consistently and generally. I only wanted to put the > information onto the list, to have it documented. It don't burden me. It's nice to read an other view on mmh. Most of the time it gives me good idea for future work or how I have to change my code to get better results. > P.S. > Not related to this topic, more in general: I really appreciate > that you supported your changes with test cases. This is good > work. It will make mmh increasingly stronger. Thank you! A test helps a lot, first it's the best argument why my patch is needed. Second I use the tests to see if I really fixed the problems. Third I see later, if another patch kills some functionality. So if anybody has a problem with mmh, a test case help me (and others) to fix this issue. Philipp
Re: [mmh] show sequences racecondition
Hoi. [2020-03-21 16:15] Philipp Takacs > [2020-03-21 09:32] markus schnalke > > [2020-03-19 12:14] Philipp Takacs > > > [2019-12-15 17:55] Philipp Takacs > > > > > > > > I have looked at the problem with open pager for show and change the > > > > sequences during the pager is open. This happen often, if you show a > > > > message and recieve a new message. > > > > This is an annoying inconvenience (actually a bug but not too > > severe) that we probably all suffer from. Improving the situation > > would be great. > > > > We can't get fully rid of the race condition, or? We have multiple > > processes writing the same file, thus we will inevitably have a > > conflict window. Correct? Could you explain quickly how we react > > in this case of conflict? > > So currently and after my path last writer winns. Currenly a programm > reads the sequences and does it work. If the work includes changing > the sequences, it writes the sequences to disk. > > My patches reads the sequences after show is finisched with the main > work. The changes to the sequences are then aplied to the new set of > sequences. This makes the conflict window smaler, but the race > condition is still there. My patch does this only for show, but I want > to add this to seq_save. Thanks for the explanation. Show(1) is the only case where I've noticed the problem. Thus it's the most important. > I found then, that seq_read and seq_list ignores mails witch it thinks > don't exist. This I have disabled this, to avoid a secound read of the > of the whole directory. > > Now the conflict window is small, but still there. The best way to > handle this is locking. This whole topic is a special one ... One case that one should have in mind as well: Stuff shouldn't mess up, if you have show(1) running while deleting/refiling/incing messages, not only to the end but possibly on previously used message numbers. For instance, I use `folder -pack' on the inbox quite often ... there might be a `repl' job in background at the same time ... I really don't want to burden you with this stuff. I think that improving the most annoying problem is already a great step forward. The rest is more of a separate topic to dig into and solve consistently and generally. I only wanted to put the information onto the list, to have it documented. meillo P.S. Not related to this topic, more in general: I really appreciate that you supported your changes with test cases. This is good work. It will make mmh increasingly stronger. Thank you!
Re: [mmh] show sequences racecondition
Hi [2020-03-21 09:32] markus schnalke > [2020-03-19 12:14] Philipp Takacs > > [2019-12-15 17:55] Philipp Takacs > > > > > > I have looked at the problem with open pager for show and change the > > > sequences during the pager is open. This happen often, if you show a > > > message and recieve a new message. > > This is an annoying inconvenience (actually a bug but not too > severe) that we probably all suffer from. Improving the situation > would be great. > > We can't get fully rid of the race condition, or? We have multiple > processes writing the same file, thus we will inevitably have a > conflict window. Correct? Could you explain quickly how we react > in this case of conflict? So currently and after my path last writer winns. Currenly a programm reads the sequences and does it work. If the work includes changing the sequences, it writes the sequences to disk. My patches reads the sequences after show is finisched with the main work. The changes to the sequences are then aplied to the new set of sequences. This makes the conflict window smaler, but the race condition is still there. My patch does this only for show, but I want to add this to seq_save. I found then, that seq_read and seq_list ignores mails witch it thinks don't exist. This I have disabled this, to avoid a secound read of the of the whole directory. Now the conflict window is small, but still there. The best way to handle this is locking. Philipp
Re: [mmh] show sequences racecondition
Hoi, good to see you being active again, Philipp. I try to put my share in as well, at least comment on your messages. So, let's start here: [2020-03-19 12:14] Philipp Takacs > [2019-12-15 17:55] Philipp Takacs > > > > I have looked at the problem with open pager for show and change the > > sequences during the pager is open. This happen often, if you show a > > message and recieve a new message. This is an annoying inconvenience (actually a bug but not too severe) that we probably all suffer from. Improving the situation would be great. We can't get fully rid of the race condition, or? We have multiple processes writing the same file, thus we will inevitably have a conflict window. Correct? Could you explain quickly how we react in this case of conflict? (For once I sadly am not able to dig into the code at the moment, but as well it often is valuable to explain the code to someone else in order to check the approach taken.) meillo
Re: [mmh] show sequences racecondition and Previous-Sequence
Hi
[2019-12-15 17:55] Philipp Takacs
> I have looked at the problem with open pager for show and change the
> sequences during the pager is open. This happen often, if you show a
> message and recieve a new message.
after a hint from simon, that in my patch is still a racecondition I have
looked at it again and found the bug. A patch is attached.
Explenation: seq_read and seq_list totaly ignore mails without the
EXISTS flag. So when a new mail arrives during show has it's pager
open, the seq_read will see the new unseen flag and ignores it. Because
it doesn't know this mail exists. To avoid a total reread for some coner
cases I belive it's better just to ignore, if a mail exists or not.
comments?
Philipp
From 9bf8d356af4e3a6ad57d7010b5088c85f91935f8 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Wed, 18 Mar 2020 19:53:25 +0100
Subject: [PATCH 3/3] seq_read and seq_list also recognise msg which don't
exist
This allows better handling of raceconditions, like a new mail gets
deliverd while a unreed mail is shown.
---
sbr/seq_list.c | 7 +++
sbr/seq_read.c | 9 +++--
test/tests/show/test-unseen-update | 13 ++---
3 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/sbr/seq_list.c b/sbr/seq_list.c
index 3606f6f3..65cc7aa0 100644
--- a/sbr/seq_list.c
+++ b/sbr/seq_list.c
@@ -54,12 +54,12 @@ seq_list(struct msgs *mp, char *seqname)
bp = buffer;
- for (i = mp->lowmsg; i <= mp->hghmsg; ++i) {
+ for (i = mp->lowmsg; i <= mp->hghoff; ++i) {
/*
** If message doesn't exist, or isn't in
** the sequence, then continue.
*/
- if (!does_exist(mp, i) || !in_sequence(mp, seqnum, i))
+ if (!in_sequence(mp, seqnum, i))
continue;
/*
@@ -91,8 +91,7 @@ seq_list(struct msgs *mp, char *seqname)
/*
** Scan to the end of this message range
*/
- for (++i; i <= mp->hghmsg && does_exist(mp, i) &&
-in_sequence(mp, seqnum, i); ++i)
+ for (++i; i <= mp->hghoff && in_sequence(mp, seqnum, i); ++i)
;
if (i - j > 1) {
diff --git a/sbr/seq_read.c b/sbr/seq_read.c
index 041f6327..4cc42233 100644
--- a/sbr/seq_read.c
+++ b/sbr/seq_read.c
@@ -210,9 +210,14 @@ seq_init(struct msgs *mp, char *name, char *field)
** We iterate through messages in this range
** and flip on bit for this sequence.
*/
+ if (k > mp->hghoff) {
+if (!(mp = folder_realloc(mp, mp->lowoff, k))) {
+ adios(EX_OSERR, NULL, "unable to allocate folder storage");
+}
+mp->hghoff = k;
+ }
for (; j <= k; j++) {
-if (j >= mp->lowmsg && j <= mp->hghmsg &&
- does_exist(mp, j))
+if (j >= mp->lowmsg && j <= mp->hghoff)
add_sequence(mp, i, j);
}
}
diff --git a/test/tests/show/test-unseen-update b/test/tests/show/test-unseen-update
index a4875c43..61e317ed 100644
--- a/test/tests/show/test-unseen-update
+++ b/test/tests/show/test-unseen-update
@@ -24,7 +24,14 @@ runandcheck "pick u" <
Re: [mmh] show sequences racecondition and Previous-Sequence
[2019-12-15 17:55] Philipp Takacs > part 1 text/plain1256 > Hi Hi, > I have looked at the problem with open pager for show and change the > sequences during the pager is open. This happen often, if you show a > message and recieve a new message. This issue hits me pretty often because I'll have open several messages at once with a single show instance, and then while reading/responding new mails get delivered and .mh_sequences gets updated and then when I'm done with the mails I'll close the pager and show overwrites the .mh_sequences thereby silently losing the unseen flag on any newly added messages. > During that I found the Previous-Sequence again. I don't see a reason > why we need this feature. The usecases, I see, for the feature are better > handled with mark or the backlog of your shell. But I don't use this > feature. [..] I don't personally use the Previous-Sequence option, so I can't comment here. > So back to show I have two commits attached. One to remove the > Previous-Sequence and one to handle the unseen sequence better. If we > decide to keep the Previous-Sequence, I have to look at the race again. > > The unseen sequence has it's own flag beside the sequences in the > msgs struct. The seq_setunseen() function updates the unseen sequence > in this struct. So I have just moved the seq_setunseen() call to the > end and added a seq_read() call before. This way it's still a bit racy > but not this hard. To fix the rest of the race we could add locking. The attached solution works well enough for me, though adding locking would of course be nice. -- Simon Thelen
[mmh] show sequences racecondition and Previous-Sequence
Hi
I have looked at the problem with open pager for show and change the
sequences during the pager is open. This happen often, if you show a
message and recieve a new message.
During that I found the Previous-Sequence again. I don't see a reason
why we need this feature. The usecases, I see, for the feature are better
handled with mark or the backlog of your shell. But I don't use this
feature.
The problem I see is, that sequences get racy, if you want to handle
your mail in parallel. I.e. have a mail open with show and get a new
mail. Does anyone see a usecase witch can't be handled with mark or the
backlog of your shell? Or an other reason to keep the Previous-Sequence?
So back to show I have two commits attached. One to remove the
Previous-Sequence and one to handle the unseen sequence better. If we
decide to keep the Previous-Sequence, I have to look at the race again.
The unseen sequence has it's own flag beside the sequences in the
msgs struct. The seq_setunseen() function updates the unseen sequence
in this struct. So I have just moved the seq_setunseen() call to the
end and added a seq_read() call before. This way it's still a bit racy
but not this hard. To fix the rest of the race we could add locking.
Comments?
Philipp
From b4b4f102566d1d3d0e4686ebf157d513a6a68f30 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Sun, 8 Dec 2019 17:54:29 +0100
Subject: [PATCH 1/2] remove previus sequence
unfinished, docs need update
---
config/config.c | 1 -
h/mh.h| 1 -
sbr/Makefile.in | 2 +-
sbr/m_draft.c | 1 -
sbr/seq_setprev.c | 44
uip/burst.c | 1 -
uip/comp.c| 1 -
uip/dist.c| 1 -
uip/folder.c | 1 -
uip/forw.c| 1 -
uip/mhlist.c | 1 -
uip/mhparam.c | 2 --
uip/mhpath.c | 2 --
uip/mhshow.c | 1 -
uip/mhstore.c | 1 -
uip/mhtest.c | 1 -
uip/packf.c | 1 -
uip/pick.c| 1 -
uip/refile.c | 2 --
uip/repl.c| 1 -
uip/rmm.c | 1 -
uip/send.c| 1 -
uip/sortm.c | 1 -
23 files changed, 1 insertion(+), 69 deletions(-)
delete mode 100644 sbr/seq_setprev.c
diff --git a/config/config.c b/config/config.c
index 5e250414..0a63912d 100644
--- a/config/config.c
+++ b/config/config.c
@@ -89,7 +89,6 @@ char *seq_unseen = "u";
char *seq_neg= "!";
char *usequence = "Unseen-Sequence";
-char *psequence = "Previous-Sequence";
char *nsequence = "Sequence-Negation";
/* profile entries for storage locations */
diff --git a/h/mh.h b/h/mh.h
index 6337a0ca..649582d3 100644
--- a/h/mh.h
+++ b/h/mh.h
@@ -288,7 +288,6 @@ extern char *msgprot;
extern char *nmhstorage;
extern char *nsequence;
extern char *profile;
-extern char *psequence;
extern char *rcvdistcomps;
extern char *replcomps;
extern char *replgroupcomps;
diff --git a/sbr/Makefile.in b/sbr/Makefile.in
index f2310fb8..142a7f20 100644
--- a/sbr/Makefile.in
+++ b/sbr/Makefile.in
@@ -68,7 +68,7 @@ SRCS = addrsbr.c ambigsw.c brkstring.c \
readconfig.c seq_add.c seq_bits.c \
seq_del.c seq_getnum.c seq_list.c seq_nameok.c \
seq_print.c seq_read.c seq_save.c seq_setcur.c \
- seq_setprev.c seq_setunseen.c signals.c \
+ seq_setunseen.c signals.c \
smatch.c snprintb.c strcasecmp.c \
strindex.c trim.c trimcpy.c uprf.c vfgets.c fmt_def.c \
mf.c utils.c m_mktemp.c seq_msgstats.c \
diff --git a/sbr/m_draft.c b/sbr/m_draft.c
index ee857900..050c7e51 100644
--- a/sbr/m_draft.c
+++ b/sbr/m_draft.c
@@ -53,7 +53,6 @@ m_draft(char *which)
*/
if (!m_convert(mp, which))
exit(EX_SOFTWARE);
- seq_setprev(mp);
snprintf(buffer, sizeof(buffer), "%s/%s", mp->foldpath,
m_name(mp->lowsel));
diff --git a/sbr/seq_setprev.c b/sbr/seq_setprev.c
deleted file mode 100644
index f4304cc5..
--- a/sbr/seq_setprev.c
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
-** seq_setprev.c -- set the Previous-Sequence
-**
-** This code is Copyright (c) 2002, by the authors of nmh. See the
-** COPYRIGHT file in the root directory of the nmh distribution for
-** complete copyright information.
-*/
-
-#include
-#include
-
-/*
-** Add all the messages currently SELECTED to
-** the Previous-Sequence. This way, when the next
-** command is given, there is a convenient way to
-** selected all the messages used in the previous
-** command.
-*/
-
-void
-seq_setprev(struct msgs *mp)
-{
- char **ap, *cp, *dp;
-
- /*
- ** Get the list of sequences for Previous-Sequence
- ** and split them.
- */
- if ((cp = context_find(psequence))) {
- dp = mh_xstrdup(cp);
- if (!(ap = brkstring(dp, " ", "\n")) || !*ap) {
- mh_free0(&dp);
- return;
- }
- } else {
- return;
- }
-
- /* Now add all SELECTED messages to each sequence */
- for (; *ap; ap++)
- seq_addsel(mp, *ap, -1, 1);
-
- mh_free0(&dp);
-}
diff --git a/uip/burst.c b/uip/burst.c
index 12d1f9d4..09a60d9f 100644
--- a/uip/burst.c
+++ b/uip/burst.c
@@ -117,7 +117,6 @@ main(int argc, char **argv)
for (msgnum = 0; msgnum
