[PATCH] Use IMAP MOVE extension if available

Post by Nikolaus Rath
Hello,
The attached revision fixes a small error when parsing the response to
the UID MOVE command. Previously, it would fail to find the COPYUID
response and fall back to using the message id to determine the UID of
the copied message.

I've incorporated this into the version I'm running locally, and will
push soon!

Eric Abrahamsen

2015-07-20 09:53:59 UTC

I'm running these patches locally, and (I think) seeing more hangs than
I used to. This only happens on first startup of Gnus. I set
toggle-debug-on-quit to t, and quit on the next hang -- see the
traceback below.

This is connecting to a dovecot server running on localhost. The first
thing this tells me is that the MOVE capability (which dovecot provides)
isn't getting noticed by Gnus -- as you noted in your previous exchange
with Lars (was there no resolution to that?). That does need to get
fixed, otherwise Gnus is missing a lot of added functionality.

The other thing is this hang. I'm not immediately inclined to blame Gnus
for this, but the fact is that this a local connection and there
shouldn't be anything stalling it. If anyone has any bright ideas...

nnimap-wait-for-response(85)
nnimap-get-response(85)
nnimap-command("UID STORE %s +FLAGS.SILENT (\\Deleted)" "")
nnimap-delete-article(nil t)
nnimap-split-incoming-mail()
nnimap-request-scan(nil "PR")
gnus-request-scan(nil (nnimap "PR" (nnimap-address "localhost")
(nnimap-user "user") (nnimap-authenticator login) (stuff omitted) (nnimap-stream network) (nnimap-inbox "INBOX")))
gnus-get-unread-articles(4 nil)
gnus-setup-news(nil 4 nil)
...bytecode omitted...
gnus-1(4 nil nil)
gnus(4)

Nikolaus Rath

2015-07-20 16:18:17 UTC

I think I've addressed Lars' comments, so for me the proper resolution
would be for someone to apply the patch :-).

Post by Eric Abrahamsen
The other thing is this hang. I'm not immediately inclined to blame Gnus
for this, but the fact is that this a local connection and there
shouldn't be anything stalling it. If anyone has any bright ideas...
nnimap-wait-for-response(85)
nnimap-get-response(85)
nnimap-command("UID STORE %s +FLAGS.SILENT (\\Deleted)" "")
nnimap-delete-article(nil t)

What's the content of the " *nnimap..." buffer at this point? (note the
leading space).

Best,
-Nikolaus

--
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Eric Abrahamsen

2015-07-27 04:36:29 UTC

I think I've addressed Lars' comments, so for me the proper resolution
would be for someone to apply the patch :-).

How does the attached patch look, instead? It's more code, but the
behavior should be cleaner.

What's the content of the " *nnimap..." buffer at this point? (note the
leading space).

It appears I wasn't seeing the problem I thought I was seeing. I traced
the login procedure (many times), and it turns out this is
`gnus-request-scan' for a server that doesn't support MOVE
(outlook.com), so that much is correct. The problem is that the server
connection often dies instantly, before the LOGIN is issued, which
throws the whole connection process off. At the very least, the wait for
response is being called on a dead connection. What's worse is that Gnus
seems to be getting confused about which server it's acting on: the
traceback above claims to be for the server "PR", but it's actually
trying to talk to the server "Outlook". I don't really understand what's
happening.

Anyway, this isn't a reason to hold up the MOVE patch, so maybe I'll
chuck in this CAPABILITY one and then do the MOVE. Sorry this is taking
so long.

Eric

Nikolaus Rath

2015-07-27 16:10:39 UTC

diff --git a/lisp/nnimap.el b/lisp/nnimap.el
index 161a6b4..f8e6e3e 100644
--- a/lisp/nnimap.el
+++ b/lisp/nnimap.el
@@ -489,11 +489,18 @@ textual parts.")
(when (functionp (nth 2 credentials))
(funcall (nth 2 credentials)))
;; See if CAPABILITY is set as part of login
- ;; response.
- (dolist (response (cddr login-result))
- (when (string= "CAPABILITY" (upcase (car response)))
- (setf (nnimap-capabilities nnimap-object)
- (mapcar #'upcase (cdr response))))))
+ ;; response. If it wasn't, specifically request
+ ;; it.
+ (when
+ (catch 'caps
+ (dolist (response (cddr login-result))
+ (when (string= "CAPABILITY" (upcase (car response)))
+ (throw 'caps (setq capabilities (cdr response)))))
+ (let ((req (nnimap-command "CAPABILITY")))
+ (when (car req)
+ (throw 'caps (setq capabilities (cdaddr req))))))
+ (setf (nnimap-capabilities nnimap-object)
+ (mapcar #'upcase capabilities))))
;; If the login failed, then forget the credentials
;; that are now possibly cached.
(dolist (host (list (nnoo-current-server 'nnimap)

This looks a lot more complex, but as far as I can tell it still
needlessy asks for capabilities if they have been supplied in the
response code of OK response instead of a in separate, untagged
response. So is it really worth the extra complexity, if in many cases
we still issue an explicit CAPABILITY command?

(For everyone unfamiliar with RFC3501, what I mean is that this:

* CAPABILITY IMAP4rev1 AND SOME MORE \r\n
1 OK Logged in\r\n

will be parsed correcly but this:

1 OK [CAPABILITY IMAP4rev1 AND SOME MORE] Logged in\r\n

will still cause Gnus to issue a separate CAPABILITY command).

Best,
-Nikolaus

--
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Eric Abrahamsen

2015-08-02 05:56:48 UTC

This looks a lot more complex, but as far as I can tell it still
needlessy asks for capabilities if they have been supplied in the
response code of OK response instead of a in separate, untagged
response. So is it really worth the extra complexity, if in many cases
we still issue an explicit CAPABILITY command?
* CAPABILITY IMAP4rev1 AND SOME MORE \r\n
1 OK Logged in\r\n
1 OK [CAPABILITY IMAP4rev1 AND SOME MORE] Logged in\r\n
will still cause Gnus to issue a separate CAPABILITY command).

Okay, I pushed the CAPABILITY patch, and the MOVE patch right after
that. Sorry this is all going so slowly!

I still think that we'll be able to come up with something that doesn't
unconditionally issue an extra CAPABILITY command. My patch was more
complex, but at least it avoided redundancy in *some* cases -- now we're
always doing the extra command.

Are you sure that `nnimap-parse-response' and `nnimap-parse-line' can't
handle the bracket case above? I haven't had time to test yet, but
looking at the code it seems like it ought to be able to handle it. If
there are multiple commonly-used formats for IMAP server responses, it
seems like we really ought to be handling them at the base level, when
parsing responses.

I'll admit I only started learning about IMAP about six months ago, so
I'm happy to take other people's word on how all this works. Out of
curiosity, is there a particular server that returns the kind of
response you note above? I suppose we should be making a list of
known-good IMAP servers that Gnus can interact happily with.

Yours,
Eric

Nikolaus Rath

2015-08-02 23:36:39 UTC

This looks a lot more complex, but as far as I can tell it still
needlessy asks for capabilities if they have been supplied in the
response code of OK response instead of a in separate, untagged
response. So is it really worth the extra complexity, if in many cases
we still issue an explicit CAPABILITY command?
* CAPABILITY IMAP4rev1 AND SOME MORE \r\n
1 OK Logged in\r\n
1 OK [CAPABILITY IMAP4rev1 AND SOME MORE] Logged in\r\n
will still cause Gnus to issue a separate CAPABILITY command).

Okay, I pushed the CAPABILITY patch, and the MOVE patch right after
that. Sorry this is all going so slowly!

Don't worry, I'm not in a rush. Luckily I can run the patches locally
until they applied upstream :-).

Post by Eric Abrahamsen
I still think that we'll be able to come up with something that doesn't
unconditionally issue an extra CAPABILITY command. My patch was more
complex, but at least it avoided redundancy in *some* cases -- now we're
always doing the extra command.

I don't understand this worry about one additional back and forth during
*login*. For me, starting Gnus takes about 2 seconds, and issuing the
CAPABILITY command takes about 160 ms. Is it really worth optimizing
this?

Post by Eric Abrahamsen
Are you sure that `nnimap-parse-response' and `nnimap-parse-line' can't
handle the bracket case above?

Depends what you mean. The data does end up in the list that's returned
by nnimap-parse-response, because (as the comment says), it just
"lightly" converts the response to a list of strings and lists of
string. The problem is the "lightly" - apparently this function
"succeeds" no matter what data you give it, even if this data completely
fails the assumptions that went into writing that function. Looking at
the code, I am not able to tell (in general) at which position the
response code is going to be. So the best I can do is just use the
location at which it ends up with my server - but I don't think that's a
good idea.

That's also the reason why I did not use nnimap-parse-response at all
for parsing the NAMESPACE command (in my other patch). In that case,
"parsing" the return value of nnimap-parse-response is actually a lot
harder than parsing the IMAP response directly. For example, the
nnimap-parse-response return value contains strings with opening parens,
but no strings with the corresponding closing parens.

Post by Eric Abrahamsen
I haven't had time to test yet, but looking at the code it seems like
it ought to be able to handle it. If there are multiple commonly-used
formats for IMAP server responses, it seems like we really ought to be
handling them at the base level, when parsing responses.

Making nnimap-parse-response into a proper parser would certainly be a
good idea, but that's probably better left as a separate patch.

Post by Eric Abrahamsen
I'll admit I only started learning about IMAP about six months ago, so
I'm happy to take other people's word on how all this works. Out of
curiosity, is there a particular server that returns the kind of
response you note above? I suppose we should be making a list of
known-good IMAP servers that Gnus can interact happily with.

This is mail.messagingengine.com, the IMAP server from
www.fastmail.com. IIRC they are using Cyrus.

Best,
-Nikolaus

--
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Eric Abrahamsen

2015-08-03 00:40:01 UTC

This looks a lot more complex, but as far as I can tell it still
needlessy asks for capabilities if they have been supplied in the
response code of OK response instead of a in separate, untagged
response. So is it really worth the extra complexity, if in many cases
we still issue an explicit CAPABILITY command?
* CAPABILITY IMAP4rev1 AND SOME MORE \r\n
1 OK Logged in\r\n
1 OK [CAPABILITY IMAP4rev1 AND SOME MORE] Logged in\r\n
will still cause Gnus to issue a separate CAPABILITY command).

Okay, I pushed the CAPABILITY patch, and the MOVE patch right after
that. Sorry this is all going so slowly!

Don't worry, I'm not in a rush. Luckily I can run the patches locally
until they applied upstream :-).

No, I guess you're right. It just irks, a little.

Post by Eric Abrahamsen
Are you sure that `nnimap-parse-response' and `nnimap-parse-line' can't
handle the bracket case above?

Depends what you mean. The data does end up in the list that's returned
by nnimap-parse-response, because (as the comment says), it just
"lightly" converts the response to a list of strings and lists of
string. The problem is the "lightly" - apparently this function
"succeeds" no matter what data you give it, even if this data completely
fails the assumptions that went into writing that function. Looking at
the code, I am not able to tell (in general) at which position the
response code is going to be. So the best I can do is just use the
location at which it ends up with my server - but I don't think that's a
good idea.
That's also the reason why I did not use nnimap-parse-response at all
for parsing the NAMESPACE command (in my other patch). In that case,
"parsing" the return value of nnimap-parse-response is actually a lot
harder than parsing the IMAP response directly. For example, the
nnimap-parse-response return value contains strings with opening parens,
but no strings with the corresponding closing parens.

Making nnimap-parse-response into a proper parser would certainly be a
good idea, but that's probably better left as a separate patch.

Oh certainly -- I'm not in any hurry to get stuck into that! But at some
point, yes, it would probably be nice.

This is mail.messagingengine.com, the IMAP server from
www.fastmail.com. IIRC they are using Cyrus.

That's pretty mainstream stuff, and I've heard Cyrus referred to as one
of the "sane" IMAP servers, so we should definitely be trying to play
nice with it...

Thanks,
Eric

Dan Christensen

2015-08-03 01:08:54 UTC

Post by Nikolaus Rath
I don't understand this worry about one additional back and forth during
*login*. For me, starting Gnus takes about 2 seconds, and issuing the
CAPABILITY command takes about 160 ms. Is it really worth optimizing
this?

I believe that Gnus has to re-login now and then, e.g. when connections
are idle for too long. Logging in to my remote IMAP server currently
takes a bit under 1s, so the above could presumably add 10 to 20% to the
time taken to open a group. If it's possible to avoid the round-trip
when it isn't needed, it seems like it would be a good idea. (I haven't
followed the details of this discussion, so I don't know how hard it
would be to implement this.)

Dan

Steinar Bang

2015-08-03 08:17:09 UTC

Post by Dan Christensen
I believe that Gnus has to re-login now and then, e.g. when connections
are idle for too long.

Ok, that may be a case for optimization: the CAPABILITIES may change
before and after login, but it is not very likely that they will change
for the rest of the session.

Ie. issue CAPABILITIES only after the initial login.

Nikolaus Rath

2015-08-03 17:18:57 UTC

Post by Dan Christensen
I believe that Gnus has to re-login now and then, e.g. when connections
are idle for too long.

Ok, that may be a case for optimization: the CAPABILITIES may change
before and after login, but it is not very likely that they will change
for the rest of the session.
Ie. issue CAPABILITIES only after the initial login.

Preserving capabilities from one connection to the next would require
some major coding effort. In that case we're still better of trying very
hard to extract the capabilities from the LOGIN response.

Best,
-Nikolaus

--
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Eric Abrahamsen

2015-08-04 03:17:49 UTC

Post by Dan Christensen
I believe that Gnus has to re-login now and then, e.g. when connections
are idle for too long.

Ok, that may be a case for optimization: the CAPABILITIES may change
before and after login, but it is not very likely that they will change
for the rest of the session.
Ie. issue CAPABILITIES only after the initial login.

I'd say we ought to leave it the way it is now, until we've got a better
parser for command responses.

E

Nikolaus Rath

2015-08-04 17:28:31 UTC

Post by Dan Christensen
I believe that Gnus has to re-login now and then, e.g. when connections
are idle for too long.

Ok, that may be a case for optimization: the CAPABILITIES may change
before and after login, but it is not very likely that they will change
for the rest of the session.
Ie. issue CAPABILITIES only after the initial login.

I'd say we ought to leave it the way it is now, until we've got a better
parser for command responses.

I hope with "the way it is now", you mean with either your or my patch
applied?

Without them, capabilities are not detected at all in many cases. I
think fixing that should not wait until the response parser is fixed.

Best,
-Nikolaus

--
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Eric Abrahamsen

2015-08-05 01:53:02 UTC