Re: nnimap versus tls

[Gaute B Strokkenes <gs234@cam.ac.uk>]

On 7 jul 2003, kai.grossjohann@gmx.net wrote:
> Gaute B Strokkenes <gs234@cam.ac.uk> writes:
>
>> (while (and (null imap-continuation)
>> -		  (memq (process-status imap-process) '(open run))
>> +		  imap-more-coming
>> +;		  (memq (process-status imap-process) '(open run))
>> 		  (< imap-reached-tag tag))
>> 	(let ((len (/ (point-max) 1024))
>> 	      message-log-max)
>> 	  (unless (< len 10)
>> 	    (setq imap-have-messaged t)
>> 	    (message "imap read: %dk" len))
>> +	  (setq imap-more-coming
>> +		(memq (process-status imap-process) '(open run)))
>> 	  (accept-process-output imap-process
>> 				 (truncate imap-read-timeout)
>> 				 (truncate (* (- imap-read-timeout
>
> Here is another idea:
>
> (setq imap-more-coming
> (or (memq (process-status imap-process) '(open run))
> (accept-process-output ...)))
>
> This means that it will do one more round of the loop if it got some
> output last time -- accept-process-output returns non-nil iff some
> output was read from the process.

Uhm, I don't quite see why that would help.  Imaging that in one
iteration we receive nothing before we timeout.  Then we received the
server's last gasp, and the status changes to exit.  Then we will miss
the last gasp.  Granted it's a small window of opportunity, but so is
the one that we're already looking at.

> But maybe it is better to just use the first, uglier, version: it
> makes it very clear what is happening, and thus people will not be
> confused.

Well, there was a problem with the prettier version--while it fixed
the problem discussed, but seemed to cause another problem, where the
imap connection would seemingly hang indefinitely.  After much
wailing, gnashing of teeth and debugging (running emacs and gnutls-cli
under strace, modifying source to add debug output etc...) I
determined that the problem seems to lie in gnutls-cli.  I have solved
this for the moment by using openssl instead.  (I might try to debug
gnutls-cli, but I am not in the mood right now.)

However, the fix as it stands is not complete.  Consider the following
(parital) backtrace:

  Debugger entered--Lisp error: (error "Selecting deleted buffer")
    imap-arrival-filter(#<process imap<1>> "*** Received corrupted data(-9) - server has terminated the connection abnormally\n")
    delete-process(#<process imap<2>>)
    imap-close(#<buffer  *imap source*>)

See?  gnutls-cli likes to spit out that line "*** Received
corrupted"... after the connection is closed.  If emacs (and thus
imap-arrival-filter) receives that line separately from the previous
one, then there is a chance that that lone line will still be awaiting
processing after imap-close returns, and so we loose.

Similarly openssl seems to like to write out "read:errno=0" when the
connection goes down.

I still think we should do the extra accept-process-output (it's
obviously the correct thing to do) but this is not enough to protect
us from imap-arrival-filter barfing.  So I suggest that we do what the
emacs lisp manual recommends and wrap the body of imap-arrival-filter
in (when (process-buffer proc) ...).

Index: lisp/imap.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/imap.el,v
retrieving revision 6.53
diff -u -r6.53 imap.el
--- lisp/imap.el	7 Jul 2003 18:44:13 -0000	6.53
+++ lisp/imap.el	8 Jul 2003 21:55:26 -0000
@@ -1754,15 +1754,13 @@
 				 (truncate (* (- imap-read-timeout
 						 (truncate imap-read-timeout))
 					      1000)))))
-      ;; Maybe the process has died, but the remote end wants to send
-      ;; some more stuff.
+      ;; A process can die _before_ we have processed everything it
+      ;; has to say.  Moreover, this can happen in between the call to
+      ;; accept-process-output and the call to process-status in an
+      ;; iteration of the loop above.
       (when (and (null imap-continuation)
 		 (< imap-reached-tag tag))
-	(accept-process-output imap-process
-			       (truncate imap-read-timeout)
-			       (truncate (* (- imap-read-timeout
-					       (truncate imap-read-timeout))
-					    1000))))
+	(accept-process-output imap-process 0 0))
       (when imap-have-messaged
 	(message ""))
       (and (memq (process-status imap-process) '(open run))
@@ -1789,34 +1787,35 @@
 
 (defun imap-arrival-filter (proc string)
   "IMAP process filter."
-  (with-current-buffer (process-buffer proc)
-    (goto-char (point-max))
-    (insert string)
-    (and imap-log
-	 (with-current-buffer (get-buffer-create imap-log-buffer)
-	   (imap-disable-multibyte)
-	   (buffer-disable-undo)
-	   (goto-char (point-max))
-	   (insert string)))
-    (let (end)
-      (goto-char (point-min))
-      (while (setq end (imap-find-next-line))
-	(save-restriction
-	  (narrow-to-region (point-min) end)
-	  (delete-backward-char (length imap-server-eol))
-	  (goto-char (point-min))
-	  (unwind-protect
-	      (cond ((eq imap-state 'initial)
-		     (imap-parse-greeting))
-		    ((or (eq imap-state 'auth)
-			 (eq imap-state 'nonauth)
-			 (eq imap-state 'selected)
-			 (eq imap-state 'examine))
-		     (imap-parse-response))
-		    (t
-		     (message "Unknown state %s in arrival filter"
-			      imap-state)))
-	    (delete-region (point-min) (point-max))))))))
+  (when (process-buffer proc)
+    (with-current-buffer (process-buffer proc)
+      (goto-char (point-max))
+      (insert string)
+      (and imap-log
+	   (with-current-buffer (get-buffer-create imap-log-buffer)
+	     (imap-disable-multibyte)
+	     (buffer-disable-undo)
+	     (goto-char (point-max))
+	     (insert string)))
+      (let (end)
+	(goto-char (point-min))
+	(while (setq end (imap-find-next-line))
+	  (save-restriction
+	    (narrow-to-region (point-min) end)
+	    (delete-backward-char (length imap-server-eol))
+	    (goto-char (point-min))
+	    (unwind-protect
+		(cond ((eq imap-state 'initial)
+		       (imap-parse-greeting))
+		      ((or (eq imap-state 'auth)
+			   (eq imap-state 'nonauth)
+			   (eq imap-state 'selected)
+			   (eq imap-state 'examine))
+		       (imap-parse-response))
+		      (t
+		       (message "Unknown state %s in arrival filter"
+				imap-state)))
+	      (delete-region (point-min) (point-max)))))))))
 
 \f
 ;; Imap parser.

However, I still feel uneasy about this.  Switching to openssl seems
to trip up the various corner cases I've been talking about a lot
less; I suppose it might be more mature implementation and so causes
much less surprises (gnutls-cli seems to like to write out individual
lines at a time, for instance).  A consequence of my switch is that I
can't really test out this stuff.

I think we really need to run this by someone who understands how this
process stuff and imap.el works (is the original author still around?)
and gain their approval.  I think it's clear that you and I do not
understand precisely how this is supposed to work.

For instance, why does imap-arrival-filter run within delete-process?
The elisp manual states that process output is only accepted when when
emacs is polling for input, or when we explicitly ask for it.  Here is
describe-function has to say about delete-process:

  delete-process is a built-in function.
  (delete-process PROCESS)

  Delete PROCESS: kill it and forget about it immediately.
  PROCESS may be a process, a buffer, the name of a process or buffer, or
  nil, indicating the current buffer's process.

I find this very unintuitive.

Also, why do we get an error at all?  The elisp manual says:

     If an error happens during execution of a filter function, it is
  caught automatically, so that it doesn't stop the execution of
  whatever program was running when the filter function was started.
  However, if `debug-on-error' is non-`nil', the error-catching is
  turned off.  This makes it possible to use the Lisp debugger to
  debug the filter function.  *Note Debugger::.

So why does this error halt gnus?  Granted I've been running with
debug-on-error set to t most of the time while debugging this, but
then why did I see this problem in the first place?  (Maybe gnus does
something special to catch errors when getting mail, (I'm thinking
about that "do you want to continue" message that one gets), I do not
know).

-- 
Gaute Strokkenes                        http://www.srcf.ucam.org/~gs234/
Oh my GOD -- the SUN just fell into YANKEE STADIUM!!