From: Matthias Dahl <ml_emacs-lists@binary-island.eu>
To: Eli Zaretskii <eliz@gnu.org>
Cc: eggert@cs.ucla.edu, larsi@gnus.org, rrandresf@gmail.com,
Stefan Monnier <monnier@iro.umontreal.ca>,
emacs-devel@gnu.org
Subject: Re: wait_reading_process_ouput hangs in certain cases (w/ patches)
Date: Tue, 26 Jun 2018 15:36:19 +0200 [thread overview]
Message-ID: <03e60e83-c832-ed6b-38bc-c0a20d2a0eac@binary-island.eu> (raw)
In-Reply-To: <2e181861-cfb8-a461-dfb6-9fee2164a611@binary-island.eu>
[-- Attachment #1: Type: text/plain, Size: 619 bytes --]
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?
Thanks a lot for taking the time,
Matthias
--
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-GnuTLS-error-handling.patch --]
[-- Type: text/x-patch; name="0001-Fix-GnuTLS-error-handling.patch", Size: 2146 bytes --]
From b6372518bb174493da482fcfb388aeb8954640dc Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Mon, 12 Mar 2018 15:33:45 +0100
Subject: [PATCH 1/3] Fix GnuTLS error handling
* src/gnutls.c (emacs_gnutls_read): All error handling should be done
in `emacs_gnutls_handle_error', move handling of
GNUTLS_E_UNEXPECTED_PACKET_LENGTH accordingly.
We always need to set `errno' in case of an error, since later error
handling (e.g. `wait_reading_process_output') depends on it and GnuTLS
does not set errno by itself. We'll otherwise have random errno values
which can cause erratic behavior and hangs.
(emacs_gnutls_handle_error): GNUTLS_E_UNEXPECTED_PACKET_LENGTH is only
returned for premature connection termination on GnuTLS < 3.0 and is
otherwise a real error and should not be gobbled up.
---
src/gnutls.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
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;
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;
+ }
}
static char const *
@@ -756,8 +758,10 @@ emacs_gnutls_handle_error (gnutls_session_t session, int err)
connection. */
# ifdef HAVE_GNUTLS3
if (err == GNUTLS_E_PREMATURE_TERMINATION)
- level = 3;
+# else
+ if (err == GNUTLS_E_UNEXPECTED_PACKET_LENGTH)
# endif
+ level = 3;
GNUTLS_LOG2 (level, max_log_level, "fatal error:", str);
ret = false;
--
2.18.0
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Always-check-GnuTLS-sessions-for-available-data.patch --]
[-- Type: text/x-patch; name="0002-Always-check-GnuTLS-sessions-for-available-data.patch", Size: 4588 bytes --]
From c52252a862c39b9877ad50640ba6deb352affd11 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Mon, 12 Mar 2018 16:07:55 +0100
Subject: [PATCH 2/3] Always check GnuTLS sessions for available data
* src/process.c (wait_reading_process_output): GnuTLS buffers data
internally and as such there is no guarantee that a select() call on
the underlying kernel socket will report available data if all data
has already been buffered. Prior to GnuTLS < 2.12, lowat mode was the
default which left bytes back in the kernel socket, so a select() call
would work. With GnuTLS >= 2.12 (the now required version for Emacs),
that default changed to non-lowat mode (and we don't set it otherwise)
and was subsequently completely removed with GnuTLS >= 3.0.
So, to properly handle GnuTLS sessions, we need to iterate through all
channels, check for available data manually and set the concerning fds
accordingly. Otherwise we might stall/delay unnecessarily or worse.
This also applies to the !just_wait_proc && wait_proc case, which was
previously handled improperly (only wait_proc was checked) and could
cause problems if sessions did have any dependency on one another
through e.g. higher up program logic and waited for one another.
---
src/process.c | 77 ++++++++++++++++-----------------------------------
1 file changed, 24 insertions(+), 53 deletions(-)
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);
+ }
+ }
}
#endif
}
--
2.18.0
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-xg_select-behave-more-like-pselect.patch --]
[-- Type: text/x-patch; name="0003-Make-xg_select-behave-more-like-pselect.patch", Size: 1322 bytes --]
From 63c4c10b9ef4fb7b40f008db5c9969357312ceae Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Tue, 13 Mar 2018 15:35:16 +0100
Subject: [PATCH 3/3] Make xg_select() behave more like pselect()
* src/xgselect.c (xg_select): If no file descriptors have data ready,
pselect() clears the passed in fd sets whereas xg_select() does not
which caused Bug#21337 for `wait_reading_process_output'.
Clear the passed in sets if no fds are ready but leave them untouched
if pselect() returns an error -- just like pselect() does itself.
---
src/xgselect.c | 8 ++++++++
1 file changed, 8 insertions(+)
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);
+ }
/* If Gtk+ is in use eventually gtk_main_iteration will be called,
unless retval is zero. */
--
2.18.0
next prev parent reply other threads:[~2018-06-26 13:36 UTC|newest]
Thread overview: 151+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-24 18:52 wait_reading_process_ouput hangs in certain cases (w/ patches) Matthias Dahl
2017-10-25 14:53 ` Eli Zaretskii
2017-10-26 14:07 ` Matthias Dahl
2017-10-26 16:23 ` Eli Zaretskii
2017-10-26 18:56 ` Matthias Dahl
2017-10-28 8:20 ` Matthias Dahl
2017-10-28 9:28 ` Eli Zaretskii
2017-10-30 9:48 ` Matthias Dahl
2017-11-03 8:52 ` Matthias Dahl
2017-11-03 9:58 ` Eli Zaretskii
2017-11-04 12:11 ` Eli Zaretskii
2017-11-06 14:15 ` Matthias Dahl
2017-11-06 16:34 ` Eli Zaretskii
2017-11-06 18:24 ` Paul Eggert
2017-11-06 20:17 ` Eli Zaretskii
2017-11-07 14:18 ` Matthias Dahl
2017-11-07 16:40 ` Eli Zaretskii
2017-11-10 14:45 ` Matthias Dahl
2017-11-10 15:25 ` Eli Zaretskii
2017-11-12 21:17 ` Paul Eggert
2017-11-13 3:27 ` Eli Zaretskii
2017-11-13 5:27 ` Paul Eggert
2017-11-13 16:00 ` Eli Zaretskii
2017-11-13 19:42 ` Paul Eggert
2017-11-13 20:12 ` Eli Zaretskii
2017-11-13 14:13 ` Matthias Dahl
2017-11-13 16:10 ` Eli Zaretskii
2017-11-14 15:05 ` Matthias Dahl
2017-11-13 19:44 ` Paul Eggert
2017-11-14 14:58 ` Matthias Dahl
2017-11-14 15:24 ` Paul Eggert
2017-11-14 16:03 ` Eli Zaretskii
2017-11-14 16:23 ` Eli Zaretskii
2017-11-14 21:54 ` Paul Eggert
2017-11-15 14:03 ` Matthias Dahl
2017-11-16 15:37 ` Eli Zaretskii
2017-11-16 16:46 ` Paul Eggert
2017-11-18 14:24 ` Matthias Dahl
2017-11-18 14:51 ` Eli Zaretskii
2017-11-18 17:14 ` Stefan Monnier
2017-11-19 7:07 ` Paul Eggert
2017-11-19 15:42 ` Eli Zaretskii
2017-11-19 17:06 ` Paul Eggert
2017-11-20 15:29 ` Matthias Dahl
2017-11-21 14:44 ` Matthias Dahl
2017-11-21 21:31 ` Clément Pit-Claudel
2017-11-22 14:14 ` Matthias Dahl
2017-11-22 8:55 ` Paul Eggert
2017-11-22 14:33 ` Matthias Dahl
2017-11-24 2:31 ` Stefan Monnier
2017-12-28 17:52 ` Eli Zaretskii
2017-12-04 9:40 ` Matthias Dahl
2018-02-13 14:25 ` Matthias Dahl
2018-02-13 16:56 ` Paul Eggert
2018-02-16 16:01 ` Eli Zaretskii
2018-02-16 16:09 ` Lars Ingebrigtsen
2018-02-16 16:54 ` Lars Ingebrigtsen
2018-02-22 11:45 ` andres.ramirez
2018-02-26 14:39 ` Matthias Dahl
2018-02-26 15:11 ` andrés ramírez
2018-02-26 15:17 ` Lars Ingebrigtsen
2018-02-26 15:29 ` andrés ramírez
2018-02-26 16:52 ` Daniel Colascione
2018-02-26 17:19 ` andrés ramírez
2018-02-26 17:24 ` Daniel Colascione
2018-02-27 1:53 ` Re: andrés ramírez
2018-02-27 9:15 ` wait_reading_process_ouput hangs in certain cases (w/ patches) Matthias Dahl
2018-02-27 12:01 ` Lars Ingebrigtsen
2018-02-27 9:11 ` Matthias Dahl
2018-02-27 11:54 ` andrés ramírez
2018-02-27 15:02 ` Matthias Dahl
2018-02-27 15:13 ` Lars Ingebrigtsen
2018-02-27 15:17 ` Matthias Dahl
2018-02-27 15:19 ` Lars Ingebrigtsen
2018-02-27 15:14 ` Matthias Dahl
2018-02-27 15:17 ` Lars Ingebrigtsen
2018-03-01 10:42 ` Lars Ingebrigtsen
2018-03-01 14:36 ` Matthias Dahl
2018-03-01 15:10 ` andrés ramírez
2018-03-01 16:30 ` T.V Raman
2018-03-01 16:46 ` andrés ramírez
2018-03-01 18:23 ` T.V Raman
2018-03-01 19:13 ` Eli Zaretskii
2018-03-02 20:21 ` andrés ramírez
2018-03-03 7:55 ` Eli Zaretskii
2018-03-05 14:43 ` Matthias Dahl
2018-03-05 14:44 ` Lars Ingebrigtsen
2018-03-05 14:54 ` Matthias Dahl
2018-03-13 9:54 ` Matthias Dahl
2018-03-13 12:35 ` Robert Pluim
2018-03-13 13:40 ` Robert Pluim
2018-03-13 15:10 ` Matthias Dahl
2018-03-13 15:30 ` Robert Pluim
2018-03-13 15:36 ` Dmitry Gutov
2018-03-13 15:46 ` Robert Pluim
2018-03-13 15:56 ` Dmitry Gutov
2018-03-13 16:57 ` Robert Pluim
2018-03-13 18:03 ` Eli Zaretskii
2018-03-13 20:12 ` Robert Pluim
2018-03-14 14:21 ` Matthias Dahl
2018-03-13 16:32 ` Lars Ingebrigtsen
2018-03-14 9:32 ` Matthias Dahl
2018-03-14 14:55 ` Lars Ingebrigtsen
2018-03-31 15:44 ` Lars Ingebrigtsen
2018-04-01 2:05 ` andrés ramírez
2018-06-08 5:11 ` Leo Liu
2018-06-08 6:57 ` Eli Zaretskii
2018-06-08 9:07 ` Leo Liu
2018-03-13 16:12 ` Eli Zaretskii
2018-03-14 4:16 ` Leo Liu
2018-03-14 9:22 ` Robert Pluim
2018-03-15 0:37 ` Leo Liu
2018-03-14 15:09 ` andrés ramírez
2018-03-14 16:45 ` Eli Zaretskii
2018-03-15 1:03 ` Leo Liu
2018-03-15 7:55 ` Robert Pluim
2018-03-14 22:54 ` Stefan Monnier
2018-03-15 1:06 ` Leo Liu
2018-03-14 9:56 ` Matthias Dahl
2018-03-14 12:24 ` Stefan Monnier
2018-03-14 14:34 ` Matthias Dahl
2018-03-14 22:52 ` Stefan Monnier
2018-03-15 15:17 ` Matthias Dahl
2018-03-14 16:43 ` Eli Zaretskii
2018-03-15 14:59 ` Matthias Dahl
2018-06-26 13:36 ` Matthias Dahl [this message]
2018-06-26 14:09 ` andrés ramírez
2018-06-27 13:10 ` Matthias Dahl
2018-06-27 15:18 ` Eli Zaretskii
2018-06-28 8:01 ` Matthias Dahl
2018-06-28 13:04 ` Eli Zaretskii
2018-06-28 13:25 ` Matthias Dahl
2018-06-28 16:33 ` Leo Liu
2018-06-28 18:31 ` T.V Raman
2018-07-02 13:27 ` Matthias Dahl
2018-07-02 14:35 ` T.V Raman
2018-07-03 13:27 ` Matthias Dahl
2018-07-03 13:52 ` T. V. Raman
2018-07-03 15:03 ` Stefan Monnier
2018-07-02 13:24 ` Matthias Dahl
2018-07-14 2:27 ` Leo Liu
2018-07-03 13:34 ` Matthias Dahl
2018-07-03 18:57 ` Eli Zaretskii
2018-07-04 7:35 ` Matthias Dahl
2018-07-04 15:11 ` Eli Zaretskii
2018-07-21 9:52 ` Eli Zaretskii
2018-08-07 8:38 ` Matthias Dahl
2018-08-07 17:10 ` Paul Eggert
2018-09-10 8:21 ` Eli Zaretskii
2017-11-07 17:23 ` Stefan Monnier
2017-11-10 14:53 ` Matthias Dahl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=03e60e83-c832-ed6b-38bc-c0a20d2a0eac@binary-island.eu \
--to=ml_emacs-lists@binary-island.eu \
--cc=eggert@cs.ucla.edu \
--cc=eliz@gnu.org \
--cc=emacs-devel@gnu.org \
--cc=larsi@gnus.org \
--cc=monnier@iro.umontreal.ca \
--cc=rrandresf@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.