Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

Hi Noah,

I have very severe problems fetching mail from my POP3 mailboxes after
the following change was installed:

2004-05-28  Noah Friedman  <friedman@splode.com>

	* process.c (Fdelete_process): Do not call remove_process.

In essense, Gnus manages to fetch one or two articles and then it stalls,
in some loop with accept-process-output and searching for \r\n in the buffer.

Undoing the above change makes it work again.

What (real world) problem was that change supposed to fix ?

Probably it must be fixed in another way that doesn't break existing code.

++kfs

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Noah Friedman]

>I have very severe problems fetching mail from my POP3 mailboxes after
>the following change was installed:
>
>2004-05-28  Noah Friedman  <friedman@splode.com>
>
>	* process.c (Fdelete_process): Do not call remove_process.
>
>In essense, Gnus manages to fetch one or two articles and then it stalls,
>in some loop with accept-process-output and searching for \r\n in the buffer.
>
>Undoing the above change makes it work again.
>
>What (real world) problem was that change supposed to fix ?
>
>Probably it must be fixed in another way that doesn't break existing code.

Damn.  I thought I had checked fairly carefully that nothing would be
affected since the call to remove_process happens anyway eventually.

I left a comment in the source that explained why I removed the call.  In
summary, calling remove_process there does 2 things wrong:

  (1) network process objects are removed from Vprocess_alist even if
      delete-exited-processes is nil
  (2) network processes never have their sentinel called a final time

Fdelete_process is called for network processes via kill_buffer_processes,
so the mechanics of cleaning up network processes is different than for
child processes, which are just signalled.

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

Noah Friedman <friedman@splode.com> writes:

> I left a comment in the source that explained why I removed the call.  In
> summary, calling remove_process there does 2 things wrong:
>
>   (1) network process objects are removed from Vprocess_alist even if
>       delete-exited-processes is nil

The doc string for delete-process says:

        Delete PROCESS: kill it and forget about it immediately.

To me 'immediately' means that the value of delete-exited-processes
does not apply here.  So I don't agree this is an error.

>   (2) network processes never have their sentinel called a final time

Right, that's a real bug.

The following patch should fix that (after undoing your change).

*** process.c	24 May 2004 09:49:49 +0200	1.429
--- process.c	31 May 2004 22:28:55 +0200
***************
*** 764,769 ****
--- 764,770 ----
      {
        XPROCESS (process)->status = Fcons (Qexit, Fcons (make_number (0), Qnil));
        XSETINT (XPROCESS (process)->tick, ++process_tick);
+       status_notify ();
      }
    else if (XINT (XPROCESS (process)->infd) >= 0)
      {


Unless you see problems with this, I'll install the patch tomorrow.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Noah Friedman]

>The doc string for delete-process says:
>
>        Delete PROCESS: kill it and forget about it immediately.
>
>To me 'immediately' means that the value of delete-exited-processes
>does not apply here.  So I don't agree this is an error.

Ok.

>>   (2) network processes never have their sentinel called a final time
>
>Right, that's a real bug.
>
>The following patch should fix that (after undoing your change).
>
>*** process.c	24 May 2004 09:49:49 +0200	1.429
>--- process.c	31 May 2004 22:28:55 +0200
>***************
>*** 764,769 ****
>--- 764,770 ----
>      {
>        XPROCESS (process)->status = Fcons (Qexit, Fcons (make_number (0), Qnil));
>        XSETINT (XPROCESS (process)->tick, ++process_tick);
>+       status_notify ();
>      }
>    else if (XINT (XPROCESS (process)->infd) >= 0)
>      {
>
>
>Unless you see problems with this, I'll install the patch tomorrow.

This was actually the first way I tried to do it, but something bothered me
about the fact that the sentinel for a network process can be called while
the buffer is still live; that's different than what happens for a
buffer with a child process and I was trying to maximize consistency.  

A program shouldn't have to know too much about how it's communicating with
another program, especially if it thinks it's communicating over a network.
For example ZenIRC has a hook to use an inferior program to make network
connections (e.g. for socks proxying) or to use open-network-stream
directly.  A sentinel still has to check for open/run or close/exit states,
but that's a trivial conditional whereas it's harder to work around the
case that sometimes a buffer still exists and sometimes it doesn't.

Why should that matter?  Well, a sentinel may attempt to reestablish a
connection if the remote end disconnected--in that case the buffer will
still exist.  But if the user kills the buffer, the client should not
attempt to reconnect to the remote server.  But if killing the buffer
invokes the sentinel before the buffer ceases to be active, how can it
tell which has happened?

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Richard Stallman]

    This was actually the first way I tried to do it, but something bothered me
    about the fact that the sentinel for a network process can be called while
    the buffer is still live;

Do you mean "while the process is still live"?  It is normal to call
the sentinel for a subprocess while its buffer is still live.
But what does this have to do with the buffer, anyway?

We could eliminate the difference between network connections and
subprocesses in this context by making Fdelete_process call
Fkill_process for network connections too.  I am not sure whether that
change is correct, but it sounds plausible.

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     This was actually the first way I tried to do it, but something bothered me
>     about the fact that the sentinel for a network process can be called while
>     the buffer is still live;
> 
> Do you mean "while the process is still live"?  It is normal to call
> the sentinel for a subprocess while its buffer is still live.
> But what does this have to do with the buffer, anyway?
> 
> We could eliminate the difference between network connections and
> subprocesses in this context by making Fdelete_process call
> Fkill_process for network connections too.  I am not sure whether that
> change is correct, but it sounds plausible.

I don't think that will work.

You cannot send a signal to a socket, so kill-process does not apply
to network processes.


IIUC, the problem is that a network process' sentinel would like to
be able to differentiate between the case where the connection is
closed because the remote end closed the connection, or because it
was closed by a call to delete-process.  

The wish is that the Lisp code does not have to treat using a network
process or a subprocess differently.

If remote closed the connection (or it was broken for other reasons)
we may want to reopen the connection in the sentinel.

If the connection was closed via an explicit call to delete-process, I
don't think the sentinel should ever try to reopen the connection!

Now, Noah says that delete-process may also be called via
kill_buffer_processes, but that only happens when we kill the process'
buffer, or when we exit emacs -- so in these cases, the sentinel should
not try to reconnect either.

So IMO, using delete-process should never cause the sentinel to try to
reopen the connection.  

The old code actually did accomplish just that -- but in a brutal way,
since the sentinel was not called after delete-process.


Now, to let the sentinel treat network processes and subprocesses
alike, Noah suggests that the sentinel could look to see if the
process' buffer is still alive.  But that breaks for network
processes.  

I think looking at the buffer is the wrong approach -- the sentinel
must look at the "status" message that it receives (see below).

With my proposed change (just call status_notify in Fdelete_process)
does indeed allow the sentinel to differentiate between a connection
closed via delete-process and by remote (or a broken connection):

The string passed to the sentinel is "finished\n" if delete-process
was called;  otherwise it is "exited abnormally with code 256".

Of course, that may be different from what is returned from a
subprocess, but the amount of Lisp code needed to treat the extra case
is minimal, so what else is needed ?

I will undo Noah's change and install my patch shortly.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Richard Stallman]

    IIUC, the problem is that a network process' sentinel would like to
    be able to differentiate between the case where the connection is
    closed because the remote end closed the connection, or because it
    was closed by a call to delete-process.  

If that is the issue, we could arrange for delete-process to call the
sentinel in a special way.  Perhaps we could have a different state
for connections closed by delete-process.

    Now, to let the sentinel treat network processes and subprocesses
    alike, Noah suggests that the sentinel could look to see if the
    process' buffer is still alive.

That is clearly the wrong criterion, since after delete-process the
buffer would often still be alive.

    The string passed to the sentinel is "finished\n" if delete-process
    was called;  otherwise it is "exited abnormally with code 256".

That general idea is ok, though using the status symbol would
be a cleaner way to distinguish.

But "finished" is the wrong way to decribe delete-process.
"deleted" would be more appropriate.

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Noah Friedman]

>    The string passed to the sentinel is "finished\n" if delete-process
>    was called;  otherwise it is "exited abnormally with code 256".
>
>That general idea is ok, though using the status symbol would
>be a cleaner way to distinguish.
>
>But "finished" is the wrong way to decribe delete-process.
>"deleted" would be more appropriate.

I'm happy with the idea of using `deleted'.

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     IIUC, the problem is that a network process' sentinel would like to
>     be able to differentiate between the case where the connection is
>     closed because the remote end closed the connection, or because it
>     was closed by a call to delete-process.  
> 
> If that is the issue, we could arrange for delete-process to call the
> sentinel in a special way.  Perhaps we could have a different state
> for connections closed by delete-process.
> 
>     Now, to let the sentinel treat network processes and subprocesses
>     alike, Noah suggests that the sentinel could look to see if the
>     process' buffer is still alive.
> 
> That is clearly the wrong criterion, since after delete-process the
> buffer would often still be alive.
> 
>     The string passed to the sentinel is "finished\n" if delete-process
>     was called;  otherwise it is "exited abnormally with code 256".
> 
> That general idea is ok, though using the status symbol would
> be a cleaner way to distinguish.

Yes.

Here, process-status returns `closed' in both cases, while
process-exit-status returns 0 after delete-process and 256 for
a broken connection.

> 
> But "finished" is the wrong way to decribe delete-process.
> "deleted" would be more appropriate.

Normally, you would use delete-process to close the connection.

So in that aspect "closed" would be better than "deleted".

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

> Richard Stallman <rms@gnu.org> writes:
> 
> > But "finished" is the wrong way to decribe delete-process.
> > "deleted" would be more appropriate.
> 
> Normally, you would use delete-process to close the connection.
> 
> So in that aspect "closed" would be better than "deleted".


To continue on this, what is the proper message to pass to the
sentinel if the connection was closed by remote peer (or broken).

Current message "exited abnormally with code 256" isn't very good, 
so perhaps we should change that as well (to what ...?)

However existing code may rely on the current message, so maybe we
need to leave it as is?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Noah Friedman]

>To continue on this, what is the proper message to pass to the
>sentinel if the connection was closed by remote peer (or broken).
>
>Current message "exited abnormally with code 256" isn't very good, 
>so perhaps we should change that as well (to what ...?)
>
>However existing code may rely on the current message, so maybe we
>need to leave it as is?

I did a web search just to see if there were any snippets of code which
actually tested for this string.  There weren't a lot, although it's
possible that program sources don't often get indexed.  None of the elisp
files I have archived, including gnu-emacs-sources archives going back
a few years, had any instances of that string literal.  ERC was just about
the only case I could find and that's one of two applications (the other
being zenirc) that prompted me to investigate the sentinel problem in the
first place.

In fact there hasn't been much reason to examine that string because the
process sentinel for network connections has never been called before in
this case; only when the remote end closes the connection would it get
called.  This bug is very old--I can reproduce it in emacs 18.59.

>> But "finished" is the wrong way to decribe delete-process.
>> "deleted" would be more appropriate.
>
>Normally, you would use delete-process to close the connection.
>
>So in that aspect "closed" would be better than "deleted".

"closed" seems ambiguous to me; either side could close a connection.
But only the local side can delete it.  I think changing the process status
signal is the most reliable indicator of who requested the disconnection.
In the meantime the reason string ought to be made more user-friendly
though I don't have any good suggestions.

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

Noah Friedman <friedman@splode.com> writes:

> >Normally, you would use delete-process to close the connection.
> >
> >So in that aspect "closed" would be better than "deleted".
> 
> "closed" seems ambiguous to me; either side could close a connection.

Ok, so let's settle on "deleted" and keep "exited abnormally ...".

> But only the local side can delete it.  I think changing the process status
> signal is the most reliable indicator of who requested the disconnection.

You can test that with process-exit-status when process-status
is closed (by either end).

> In the meantime the reason string ought to be made more user-friendly
> though I don't have any good suggestions.

I think we'll leave it as is, ie. if message is "exited(something)"
it was closed by remote.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Richard Stallman]

    To continue on this, what is the proper message to pass to the
    sentinel if the connection was closed by remote peer (or broken).

    Current message "exited abnormally with code 256" isn't very good, 
    so perhaps we should change that as well (to what ...?)

"connection broken by remote peer" looks like a good message to me.

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     To continue on this, what is the proper message to pass to the
>     sentinel if the connection was closed by remote peer (or broken).
> 
>     Current message "exited abnormally with code 256" isn't very good, 
>     so perhaps we should change that as well (to what ...?)
> 
> "connection broken by remote peer" looks like a good message to me.

I installed that change, and changed the local exit message to "deleted"
as previously agreed.

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Richard Stallman]

    Here, process-status returns `closed' in both cases, while
    process-exit-status returns 0 after delete-process and 256 for
    a broken connection.

Noah, does that offer your code sufficient info to DTRT?

Re: Last change to process.c breaks fetching pop3 mail (gnus/pop3.el)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     Here, process-status returns `closed' in both cases, while
>     process-exit-status returns 0 after delete-process and 256 for
>     a broken connection.
> 

We should add this information to the manual.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk