Re: wait_reading_process_ouput hangs in certain cases (w/ patches)

[Eli Zaretskii <eliz@gnu.org>]

> Cc: larsi@gnus.org, rrandresf@gmail.com, emacs-devel@gnu.org,
>  eggert@cs.ucla.edu, Stefan Monnier <monnier@iro.umontreal.ca>
> From: Matthias Dahl <ml_emacs-lists@binary-island.eu>
> Date: Tue, 26 Jun 2018 15:36:19 +0200
> 
> Hello Eli,
> 
> sorry that it has been a while since my last sign of life but 2018 has
> been an especially bad year thus far health-wise, so I am struggling
> to juggle "everything".
> 
> I just wanted to bring the attached fixes back into discussion to get
> them into master.
> 
> As I understand it, they don't completely fix the problems Andres and
> Lars have been seeing, but they still fix real bugs that can cause
> random erratic / buggy behavior and/or freezes.
> 
> What do you (and all the others in cc' :P) say?

Sadly, none of "the others" chimed in, and I'm definitely not an
expert on this stuff.  So just some comments, hopefully they will make
sense.

> diff --git a/src/gnutls.c b/src/gnutls.c
> index d22d5d267c..5bf5ee0e5c 100644
> --- a/src/gnutls.c
> +++ b/src/gnutls.c
> @@ -708,16 +708,18 @@ emacs_gnutls_read (struct Lisp_Process *proc, char *buf, ptrdiff_t nbyte)
>    rtnval = gnutls_record_recv (state, buf, nbyte);
>    if (rtnval >= 0)
>      return rtnval;
> -  else if (rtnval == GNUTLS_E_UNEXPECTED_PACKET_LENGTH)
> -    /* The peer closed the connection. */
> -    return 0;

Why is this hunk being deleted?  I see that you intend to handle it in
emacs_gnutls_handle_error, but I'm not sure I understand the net
result.  The current code simply ignores this situation; what will
happen with the new code, except for the problem being logged?

>    else if (emacs_gnutls_handle_error (state, rtnval))
> -    /* non-fatal error */
> -    return -1;
> -  else {
> -    /* a fatal error occurred */
> -    return 0;
> -  }
> +    {
> +      /* non-fatal error */
> +      errno = EAGAIN;
> +      return -1;
> +    }
> +  else
> +    {
> +      /* a fatal error occurred */
> +      errno = EPROTO;
> +      return 0;
> +    }

EPROTO is not universally available, AFAIK.  We have Gnulib's errno.h
that defines a value for it, but that just gets us through
compilation.  What do you expect this value to cause when it is
encountered by some code which needs to interpret it?  We need to make
sure the results will be sensible.

> diff --git a/src/process.c b/src/process.c
> index 6dba218c90..5702408985 100644
> --- a/src/process.c
> +++ b/src/process.c
> @@ -5397,60 +5397,31 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
>  #endif	/* !HAVE_GLIB */
>  
>  #ifdef HAVE_GNUTLS
> -          /* GnuTLS buffers data internally.  In lowat mode it leaves
> -             some data in the TCP buffers so that select works, but
> -             with custom pull/push functions we need to check if some
> -             data is available in the buffers manually.  */
> -          if (nfds == 0)
> +          /* GnuTLS buffers data internally. select() will only report
> +             available data for the underlying kernel sockets API, not
> +             what has been buffered internally. As such, we need to loop
> +             through all channels and check for available data manually.  */
> +          if (nfds >= 0)
>  	    {
> -	      fd_set tls_available;
> -	      int set = 0;
> -
> -	      FD_ZERO (&tls_available);
> -	      if (! wait_proc)
> -		{
> -		  /* We're not waiting on a specific process, so loop
> -		     through all the channels and check for data.
> -		     This is a workaround needed for some versions of
> -		     the gnutls library -- 2.12.14 has been confirmed
> -		     to need it.  See
> -		     http://comments.gmane.org/gmane.emacs.devel/145074 */
> -		  for (channel = 0; channel < FD_SETSIZE; ++channel)
> -		    if (! NILP (chan_process[channel]))
> -		      {
> -			struct Lisp_Process *p =
> -			  XPROCESS (chan_process[channel]);
> -			if (p && p->gnutls_p && p->gnutls_state
> -			    && ((emacs_gnutls_record_check_pending
> -				 (p->gnutls_state))
> -				> 0))
> -			  {
> -			    nfds++;
> -			    eassert (p->infd == channel);
> -			    FD_SET (p->infd, &tls_available);
> -			    set++;
> -			  }
> -		      }
> -		}
> -	      else
> -		{
> -		  /* Check this specific channel.  */
> -		  if (wait_proc->gnutls_p /* Check for valid process.  */
> -		      && wait_proc->gnutls_state
> -		      /* Do we have pending data?  */
> -		      && ((emacs_gnutls_record_check_pending
> -			   (wait_proc->gnutls_state))
> -			  > 0))
> -		    {
> -		      nfds = 1;
> -		      eassert (0 <= wait_proc->infd);
> -		      /* Set to Available.  */
> -		      FD_SET (wait_proc->infd, &tls_available);
> -		      set++;
> -		    }
> -		}
> -	      if (set)
> -		Available = tls_available;
> +              for (channel = 0; channel < FD_SETSIZE; ++channel)
> +                if (! NILP (chan_process[channel]))
> +                  {
> +                    struct Lisp_Process *p =
> +                      XPROCESS (chan_process[channel]);
> +
> +                    if (just_wait_proc && p != wait_proc)
> +                      continue;
> +
> +                    if (p && p->gnutls_p && p->gnutls_state
> +                        && ((emacs_gnutls_record_check_pending
> +                             (p->gnutls_state))
> +                            > 0))
> +                      {
> +                        nfds++;
> +                        eassert (p->infd == channel);
> +                        FD_SET (p->infd, &Available);
> +                      }
> +                  }

This change is hard to read.  The original code already called
emacs_gnutls_record_check_pending, and there's no calls to pselect in
the hunk, so I'm unsure what exactly are we changing here, in terms of
the details.  The overview in the commit log just gives the general
idea, but its hard for me to connect that to the actual code changes.
Plus, you lose some of the comments, for some reason, even though the
same code is still present.

Bottom line, I'd appreciate more details for this part.

> diff --git a/src/xgselect.c b/src/xgselect.c
> index fedd3127ef..f68982143e 100644
> --- a/src/xgselect.c
> +++ b/src/xgselect.c
> @@ -143,6 +143,14 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
>              ++retval;
>          }
>      }
> +  else if (nfds == 0)
> +    {
> +      // pselect() clears the file descriptor sets if no fd is ready (but
> +      // not if an error occurred), so should we to be compatible. (Bug#21337)
> +      if (rfds) FD_ZERO (rfds);
> +      if (wfds) FD_ZERO (wfds);
> +      if (efds) FD_ZERO (efds);
> +    }

This is OK, but please don't use C++ style comments, it's not our
style.  Also, please be sure to test this when waiting on pselect is
interrupted by C-g, we had some problems in that area which had roots
in xg_select.

Thanks.