bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

If I have the following in my Gnus dribble file

(gnus-group-set-info '("nntp+news.gmane.org:gwene.org.jwz.blog.comments" 3 ((1 . 25172)) ((unexist) (seen (1 . 1032) (1044 . 5020) (5022 . 11580) (11629 . 15722) (15725 . 19758) (19774 . 25172))) "nntp:news.gmane.org"))

and start Gnus, then the group buffer will then always show the same
unread count as existed in the previous Gnus session.  Entering the
group and marking everything as read has no effect -- the group still
shows the old number of unread articles.


In GNU Emacs 27.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2019-06-23 built on stories
Repository revision: abf7d0d802728e0ae8e064683eb5fb411bad911e
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9 (stretch)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> If I have the following in my Gnus dribble file
>
> (gnus-group-set-info
> '("nntp+news.gmane.org:gwene.org.jwz.blog.comments" 3 ((1 . 25172))
> ((unexist) (seen (1 . 1032) (1044 . 5020) (5022 . 11580) (11629 .
> 15722) (15725 . 19758) (19774 . 25172))) "nntp:news.gmane.org"))
>
> and start Gnus, then the group buffer will then always show the same
> unread count as existed in the previous Gnus session.  Entering the
> group and marking everything as read has no effect -- the group still
> shows the old number of unread articles.
>
>
> In GNU Emacs 27.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
>  of 2019-06-23 built on stories
> Repository revision: abf7d0d802728e0ae8e064683eb5fb411bad911e

Would you be willing to build an earlier emacs, from before c1b63af44,
and see if this is something I did?

Thanks,
Eric

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Would you be willing to build an earlier emacs, from before c1b63af44,
> and see if this is something I did?

Sure; but I'm pretty sure that this started happening after the hash
table rewrite, so perhaps it's just less work to try to debug it now.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Would you be willing to build an earlier emacs, from before c1b63af44,
>> and see if this is something I did?
>
> Sure; but I'm pretty sure that this started happening after the hash
> table rewrite, so perhaps it's just less work to try to debug it now.  :-)

:(

Okay, give me a day or so...

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Deus Max]

On Sun, Jun 23 2019, Lars Ingebrigtsen wrote:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Would you be willing to build an earlier emacs, from before c1b63af44,
>> and see if this is something I did?
>
> Sure; but I'm pretty sure that this started happening after the hash
> table rewrite, so perhaps it's just less work to try to debug it now.  :-)

What was the hash-table rewrite and when was it ?
Is there any info on it ?

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

Deus Max <deusmax@gmx.com> writes:

> On Sun, Jun 23 2019, Lars Ingebrigtsen wrote:
>
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> Would you be willing to build an earlier emacs, from before c1b63af44,
>>> and see if this is something I did?
>>
>> Sure; but I'm pretty sure that this started happening after the hash
>> table rewrite, so perhaps it's just less work to try to debug it now.  :-)
>
> What was the hash-table rewrite and when was it ?
> Is there any info on it ?

It started with the commit I mentioned above, included several patches
to master, and (I hope) will be concluded with the scratch branch you
helped me test. I'm pretty sure I just figured out what's wrong with the
dribble file, but I'm setting out on a 80-mile bike ride now, and will
fix it if/when I have survived that.

Eric

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I'm pretty sure I just figured out what's wrong with the dribble file,
> but I'm setting out on a 80-mile bike ride now, and will fix it
> if/when I have survived that.

Good luck!

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

On 06/23/19 20:40 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I'm pretty sure I just figured out what's wrong with the dribble file,
>> but I'm setting out on a 80-mile bike ride now, and will fix it
>> if/when I have survived that.
>
> Good luck!

I survived! But I'm having trouble reproducing this. Can you give me a
more basic recipe, like "mark some messages read, exit with `Q`,
restart"?

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

>>> I'm pretty sure I just figured out what's wrong with the dribble file,
>>> but I'm setting out on a 80-mile bike ride now, and will fix it
>>> if/when I have survived that.
>>
>> Good luck!
>
> I survived!

Yay!

> But I'm having trouble reproducing this. Can you give me a more basic
> recipe, like "mark some messages read, exit with `Q`, restart"?

First save with `s' so you have a "known good state".  Then enter a
group (that has some unread articles), mark everything as read (with
`d'), `q' out of the group and then `Q' out of Emacs.  You should now
have a dribble file.  Then say `M-x gnus' and answer `y' to the question
about whether to read it.

The group where you marked everything as read should now be displayed in
the group buffer with the same number of unread articles as it had
before you entered it the last time.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>>>> I'm pretty sure I just figured out what's wrong with the dribble file,
>>>> but I'm setting out on a 80-mile bike ride now, and will fix it
>>>> if/when I have survived that.
>>>
>>> Good luck!
>>
>> I survived!
>
> Yay!
>
>> But I'm having trouble reproducing this. Can you give me a more basic
>> recipe, like "mark some messages read, exit with `Q`, restart"?
>
> First save with `s' so you have a "known good state".  Then enter a
> group (that has some unread articles), mark everything as read (with
> `d'), `q' out of the group and then `Q' out of Emacs.  You should now
> have a dribble file.  Then say `M-x gnus' and answer `y' to the question
> about whether to read it.
>
> The group where you marked everything as read should now be displayed in
> the group buffer with the same number of unread articles as it had
> before you entered it the last time.

Man, this is killing me. I've narrowed it down to:

- If a dribble file is present, "g" (including gnus restart) ignores the
  dribble data, but "M-g" honors it. You can bounce back and forth
  between correct and incorrect unread counts by alternating "g" and
  "M-g".
- The dribble file is *only* eval'ed at startup time, which must
  mean that there are good and bad "versions" of group info data
  floating around, and these two commands access different versions.
- Only goes wrong for nntp groups
- Once you hit "s", everything is fine (and the dribble file is
  deleted).
- The code appears to be confused not about which articles are read, but
  how many/which articles exist in the group. 


Both "g" and "M-g" will eventually arrive at
`gnus-get-unread-articles-in-group', and in the "g" case the info and
active numbers are already wrong at that point. My current best guess is
that, in the "M-g" case, the code will issue a 'request-scan call first,
which updates the "real" active numbers for the group. nntp has no
`nntp-request-scan' function (while nnimap, nnmaildir, etc, do), which
might explain why the bug only shows up for nntp.

So then the remaining mysteries are why does "s" clear it up, and what
in my code changes could have caused this? Since I was mostly messing
with the hash tables, I have to assume that data was supposed to be
stored in some table, but isn't being retrieved correctly.

Anyway, I'm just thinking out loud, I know what the next debugging steps
are. I found another minor dribble-related bug, and will push a fix for
that in a bit.

Eric

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> So then the remaining mysteries are why does "s" clear it up, and what
> in my code changes could have caused this? Since I was mostly messing
> with the hash tables, I have to assume that data was supposed to be
> stored in some table, but isn't being retrieved correctly.

Yeah, that would also be my guess...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

On 07/04/19 15:43 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> So then the remaining mysteries are why does "s" clear it up, and what
>> in my code changes could have caused this? Since I was mostly messing
>> with the hash tables, I have to assume that data was supposed to be
>> stored in some table, but isn't being retrieved correctly.
>
> Yeah, that would also be my guess...

Got it: when `gnus-group-set-info' reaches:

(setcar (nthcdr 1 entry) info)

Ie, it actually does the setting of the info based on the dribble file,
that change is made in `gnus-newsrc-hashtb' but not `gnus-newsrc-alist'.
"g" uses the alist, "M-g" uses the hashtable. So that explains that.

I don't know why that happens, given that the entry in the hashtable and
the alist are identical lisp objects, or at least they are everywhere
else. But at least the solution is in sight!

Eric

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I don't know why that happens, given that the entry in the hashtable and
> the alist are identical lisp objects, or at least they are everywhere
> else. But at least the solution is in sight!

Great.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I don't know why that happens, given that the entry in the hashtable and
>> the alist are identical lisp objects, or at least they are everywhere
>> else. But at least the solution is in sight!
>
> Great.  :-)

I think it has to be done the ugly way -- setting the hashtable and
alist separately.

The problem (I write this more as part of digesting the issue than
anything else) is that the alist is full of elements that look like
'("group" 4 ((1 2 3) (4 5 6))), whereas the corresponding value in the
hashtable looks like '(25 ("group" 4 ((1 2 3) (4 5 6)))).

The alist element is `eq' to the `cadr' of the hashtable value: they're
the same list object.

If you reach inside that list and change values (ie set 4 to 7), they
remain the same list, and the change is reflected in both hashtable and
alist.

If you replace the entire ("group"...) list, in the hashtable, it is no
longer the same object as that in the alist, and you get divergence.
This seems sensible in hindsight, but took me quite a while to figure
out.

I think `gnus-group-set-info' is the only place that happens, so it
isn't too terrible to just explicitly set both hashtable and alist in
that function. I've attached the commit that does that.

My plan for avoiding this class of errors in the future is to change the
representation of Gnus groups from lists to EIEIO objects. Then
`gnus-newsrc-alist' would merely be a disk serialization format, and the
hashtable would be the source of authority. That would also make the
"dummy.group" unnecessary. But let's see if I get there, and if the
changes are accepted...

Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-sure-gnus-group-set-info-sets-both-the-hashtabl.patch --]
[-- Type: text/x-patch, Size: 1377 bytes --]

From c0b8589b1686d8488b4765783536217b7bb77b6c Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Sun, 7 Jul 2019 16:25:14 -0700
Subject: [PATCH] Make sure gnus-group-set-info sets both the hashtable and
 alist

* lisp/gnus/gnus-group.el (gnus-group-set-info): Apparently this
  method of updating the group info will only apply to the
  gnus-newsrc-hashtb, not gnus-newsrc-alist. Do the alist explicitly.
---
 lisp/gnus/gnus-group.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lisp/gnus/gnus-group.el b/lisp/gnus/gnus-group.el
index ea695aaa97..2668e4fb7c 100644
--- a/lisp/gnus/gnus-group.el
+++ b/lisp/gnus/gnus-group.el
@@ -4506,7 +4506,14 @@ gnus-group-set-info
 	    (when (and (not (eq (car entry) t))
 		       (gnus-active (gnus-info-group info)))
 	      (setcar entry (length
-			     (gnus-list-of-unread-articles (car info))))))
+			     (gnus-list-of-unread-articles (car info)))))
+	    ;; The above `setcar' will only affect the hashtable, not
+	    ;; the alist: update the alist separately.
+	    (push info (cdr (setq gnus-newsrc-alist
+				   (remove (assoc-string
+					    (gnus-info-group info)
+					    gnus-newsrc-alist)
+					   gnus-newsrc-alist)))))
 	(error "No such group: %s" (gnus-info-group info))))))
 
 ;; Ad-hoc function for inserting data from a different newsrc.eld
-- 
2.22.0

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I think `gnus-group-set-info' is the only place that happens, so it
> isn't too terrible to just explicitly set both hashtable and alist in
> that function. I've attached the commit that does that.

Sounds good -- please apply.

> My plan for avoiding this class of errors in the future is to change the
> representation of Gnus groups from lists to EIEIO objects. Then
> `gnus-newsrc-alist' would merely be a disk serialization format, and the
> hashtable would be the source of authority. That would also make the
> "dummy.group" unnecessary. But let's see if I get there, and if the
> changes are accepted...

The whole point of that awkward structure is to allow
inserting/removing/updating groups from the list-of-subscribed-groups as
an O(1) operation.  Updating is still fine as O(1) with just a hash
table, but without the point-at-the-previous-element list, you can't
remove the elements, or add new elements before the group, as an O(1)
thing.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

On 07/08/19 18:22 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I think `gnus-group-set-info' is the only place that happens, so it
>> isn't too terrible to just explicitly set both hashtable and alist in
>> that function. I've attached the commit that does that.
>
> Sounds good -- please apply.

Cool, will do.

>> My plan for avoiding this class of errors in the future is to change the
>> representation of Gnus groups from lists to EIEIO objects. Then
>> `gnus-newsrc-alist' would merely be a disk serialization format, and the
>> hashtable would be the source of authority. That would also make the
>> "dummy.group" unnecessary. But let's see if I get there, and if the
>> changes are accepted...
>
> The whole point of that awkward structure is to allow
> inserting/removing/updating groups from the list-of-subscribed-groups as
> an O(1) operation.  Updating is still fine as O(1) with just a hash
> table, but without the point-at-the-previous-element list, you can't
> remove the elements, or add new elements before the group, as an O(1)
> thing.

Okay, I get that. But I wonder how important it is that add/delete
operations be so efficient? Does subscription/unsubscription happen so
often that it needs to be fast? As for sorting, in current master code
sort-order is kept in `gnus-group-list' (when topic mode is off) and
`gnus-topic-alist' (when it's on), so we're already sorting using plain
lists of strings.

In fact, right at the moment, there's already no need for the ordering
in `gnus-newsrc-alist'. All of the "(while (setq info (pop newsrc))"
loops could be replaced right now with "(dolist (g gnus-group-list)
(setq group (gnus-get-info g))"

Eric

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Okay, I get that. But I wonder how important it is that add/delete
> operations be so efficient? Does subscription/unsubscription happen so
> often that it needs to be fast?

It's annoying if, say, killing ten groups takes a long time (and the
user may have tens of thousands of subscribed groups)...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

On 07/08/19 19:37 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Okay, I get that. But I wonder how important it is that add/delete
>> operations be so efficient? Does subscription/unsubscription happen so
>> often that it needs to be fast?
>
> It's annoying if, say, killing ten groups takes a long time (and the
> user may have tens of thousands of subscribed groups)...

Okay, I am happy to look at that part of the code and see if we can do
the deletions/insertions in bulk, after the group levels have been
changed. Either way, I don't think this has to be a serious speed
breaker for Gnus.

bug#36341: 27.0.50; Reading from the Gnus dribble file leaves data inconsistent

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 07/08/19 19:37 PM, Lars Ingebrigtsen wrote:
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> Okay, I get that. But I wonder how important it is that add/delete
>>> operations be so efficient? Does subscription/unsubscription happen so
>>> often that it needs to be fast?
>>
>> It's annoying if, say, killing ten groups takes a long time (and the
>> user may have tens of thousands of subscribed groups)...
>
> Okay, I am happy to look at that part of the code and see if we can do
> the deletions/insertions in bulk, after the group levels have been
> changed. Either way, I don't think this has to be a serious speed
> breaker for Gnus.

And I'm going to close this particular report for now, and go back to
working on completion of the larger patch set.