bug#36362: New feature-x-check-server

bug#36362: New feature-x-check-server

[otadmor .]

[-- Attachment #1.1: Type: text/plain, Size: 117 bytes --]

A new native method to check connectivity of xserver.

-- 
Gretz,
Ofir Tadmor

ICQ: 77685691
Mail: otadmor@gmail.com

[-- Attachment #1.2: Type: text/html, Size: 316 bytes --]

[-- Attachment #2: 0001-Add-x-check-frame-lisp-method-to-check-frame-x-serve.patch --]
[-- Type: application/octet-stream, Size: 3918 bytes --]

From 4449281873f139055fde82a8a55310a274c7e5a7 Mon Sep 17 00:00:00 2001
From: otadmor <otadmor@gmail.com>
Date: Mon, 24 Jun 2019 19:43:50 +0300
Subject: [PATCH] Add x-check-frame lisp method to check frame x-server's
 connection.

---
 src/xterm.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/src/xterm.c b/src/xterm.c
index 1acff2af0d..01ff790831 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -235,6 +235,117 @@ #define XtNinitialState "initialState"
 
 static bool x_get_current_wm_state (struct frame *, Window, int *, bool *);
 
+
+#define CLOCKID CLOCK_REALTIME
+#define CONN_SIG (SIGRTMAX-1)
+#include <signal.h>
+#include "X11/Xlibint.h"
+
+static int connection_fd = -1;
+void
+connection_timeout_handler (int signum)
+{
+    if (connection_fd != -1) {
+        close(connection_fd);
+        connection_fd = -1;
+    }
+}
+Status
+XCheckConnection (
+    register Display *dpy,
+    Drawable d, int fd, double timeout)
+{
+
+    xGetGeometryReply rep;
+    register xResourceReq *req;
+    LockDisplay(dpy);
+    GetResReq(GetGeometry, d, req);
+    Status res = 0;
+
+    sigset_t origmask;
+    sigset_t sigmask;
+    sigemptyset(&sigmask);
+    pthread_sigmask(SIG_SETMASK, &sigmask, &origmask);
+
+    // Create the timer
+    timer_t timerid;
+    struct sigevent sev;
+    struct itimerspec its;
+    long long freq_nanosecs;
+
+    struct sigaction new_action, old_action;
+
+    new_action.sa_handler = connection_timeout_handler;
+    sigemptyset (&new_action.sa_mask);
+    new_action.sa_flags = 0;
+
+    sigaction (CONN_SIG, &new_action, &old_action);
+
+    sev.sigev_notify = SIGEV_SIGNAL;
+    sev.sigev_signo = CONN_SIG;
+    sev.sigev_value.sival_ptr = &timerid;
+    (void) timer_create(CLOCKID, &sev, &timerid);
+
+    freq_nanosecs  = (long long)(timeout * 1000000000);
+    its.it_value.tv_sec  = freq_nanosecs / 1000000000;
+    its.it_value.tv_nsec = freq_nanosecs % 1000000000;
+    its.it_interval.tv_sec = 0;
+    its.it_interval.tv_nsec = 0;
+
+    (void) timer_settime(timerid, 0, &its, NULL);
+    connection_fd = fd;
+
+    res = _XReply (dpy, (xReply *)&rep, 0, xTrue);
+
+    connection_fd = -1;
+    (void) timer_delete(timerid);
+    pthread_sigmask(SIG_SETMASK, &origmask, NULL);
+    sigaction (CONN_SIG, &old_action, NULL);
+
+    UnlockDisplay(dpy);
+    SyncHandle();
+    return (int)res;
+}
+
+int check_frame_connection(struct frame * frame, double timeout)
+{
+    block_input ();
+    int fd = ConnectionNumber (FRAME_X_DISPLAY (frame));
+    int res = XCheckConnection (
+        FRAME_X_DISPLAY (frame), FRAME_OUTER_WINDOW (frame), fd, timeout);
+    unblock_input ();
+    return res;
+}
+
+DEFUN ("x-check-frame", Fx_check_frame,
+       Sx_check_frame, 1, 1, 0,
+       doc: /* check the connection with the display of the given frame.  */)
+  (Lisp_Object f)
+{
+    if (!f)
+        return Qnil;
+
+    if (NILP (f))
+        return Qnil;
+
+    if (Fframep(f) != Qx)
+        return Qnil;
+
+    if (!FLOATP (Vx_check_frame_timeout))
+        return Qnil;
+
+    /* Default timeout is 0.5 second. This timeout will occur
+       only when the libx11 did not noticed the frame was
+       disconnected. */
+    double timeout = XFLOAT_DATA (Vx_check_frame_timeout);
+
+    struct frame *frame = XFRAME (f);
+    int ret = check_frame_connection (frame, timeout);
+    return ret ? Qt : Qnil;
+}
+
+
+
 /* Flush display of frame F.  */
 
 static void
@@ -13641,4 +13752,11 @@ syms_of_xterm (void)
 consuming frame position adjustments.  In newer versions of GTK, Emacs
 always uses gtk_window_move and ignores the value of this variable.  */);
   x_gtk_use_window_move = true;
+
+  defsubr (&Sx_check_frame);
+
+  DEFVAR_LISP ("x-check-frame-timeout", Vx_check_frame_timeout,
+    doc: /* How long to wait a display to response. */);
+  Vx_check_frame_timeout = make_float (0.5);
+
 }
-- 
2.17.1

bug#36362: Patch for the current 26.2 version

[otadmor .]

[-- Attachment #1: Type: text/plain, Size: 184 bytes --]

Patch for version 26.2 can be found here:
https://raw.githubusercontent.com/otadmor/emacs-conf/master/xterm.c.patch
Patch for the current master branch in git on the original message.

[-- Attachment #2: Type: text/html, Size: 341 bytes --]

bug#36362: New feature-x-check-server

[Noam Postavsky]

"otadmor ." <otadmor@gmail.com> writes:

> A new native method to check connectivity of xserver.

Can you explain a bit when/why this is useful?  As it stands this patch
is rather "apropos of nothing"...

bug#36362: New feature-x-check-server

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 82 bytes --]

[forwarding to list, please use "Reply All" to keep 36362@debbugs.gnu.org on Cc]


[-- Attachment #2: Type: message/rfc822, Size: 2850 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 2284 bytes --]

I have some non consistency of the terms client and server. Some refer to
the emacs client/daemon, some refer to the xserver, emacs as x11 client.

Hi,
I have this scenerio:
1. Laptop with Windows 10 host and Ubuntu 1804 vm.putty and xming (latest
versions) are installed on host and emacs 26.2 on the vm.
2. Connect with putty via sshx to the vm and run emacsclient -c. Doest
really matter how emacs deamon is started (with the ex, via the ssh as
emacs -daemon, directly in the vm, as sysctl -user start emacs).
3. Close the laptop lid so it goes to sleep.
4. Open the lid and reconnect with sshx and emacs client -c.
5. The second emacs client causes emacs daemon to stuck on xcb select (they
have no timeout). The emacs client is still running correctly before the
second emacs client.

The propose is to have this connectivity check native function + timer
which executes it on all existing frames every few seconds.

This native function closes the fd of the xcb and causes the select to
return EINTR. xcb have internal infinate loop Incase of EINTR, so closing
the fd is necessary to get out of this infinite loop. Closing the fd also
causes libx11 to realize the connection was closed and call the error
handler of emacs for x11 failures for a clean termination of the resources
in emacs.

This native function is consist of a a native timer with a signal handler
to handle when this timer expires. I have set the native timer for 0.5
seconds and it is configurable using an elisp variable. When the server was
not disconnected this timer don't expires, doesnt close the fd and returned
immediately without waiting 0.5 seconds. When the client crashed because of
sleep, the native code will have to wait before realizing it. I have
selected GetGeomerty arbitrarly. This is usually used by emacs and I don't
believe it has an extra side effect. The xcb knows the sequence numbers of
the requests and should not be confused with other GetGeomerty/other
non-GetGeomerty requests.

Shalom.

On Thu, Jun 27, 2019, 18:10 Noam Postavsky <npostavs@gmail.com> wrote:

> "otadmor ." <otadmor@gmail.com> writes:
>
> > A new native method to check connectivity of xserver.
>
> Can you explain a bit when/why this is useful?  As it stands this patch
> is rather "apropos of nothing"...
>
>

bug#36362: New feature-x-check-server

[Lars Ingebrigtsen]

> This native function closes the fd of the xcb and causes the select to
> return EINTR. xcb have internal infinate loop Incase of EINTR, so closing
> the fd is necessary to get out of this infinite loop. Closing the fd also
> causes libx11 to realize the connection was closed and call the error
> handler of emacs for x11 failures for a clean termination of the resources
> in emacs.

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

I don't think it would be appropriate to add a function like this to the
C level of Emacs, because the use case is very limited.  And I think you
can basically get the same functionality by using the xelb package
(which talks to the X servers from Lisp).

So I'm closing this bug report as a "wontfix".

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36362: New feature-x-check-server

[otadmor]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

When running xlb function the code gets stuck in a native endless loop. The
patch I have added closes the fd of the xserver, which as a side effect
ends the endless loop. Some would say this patch is fixing a bug of a
dependency of emacs and not emacs itself (it is just that emacs uses it in
a certain way...).
This solution uses native timer (using signals) to detect the timeout. Upon
reaching a timeout it closes an the fd on the same thread as the xlb code
(this is because of how signals works).
To do this is lisp we need to answer the following:
1. How to find the fd of the current xserver using lisp?
2. How to call syscall close using lisp?
3. How to create native timers using lisp?
4. Is it even allowed to run lisp code while the main thread is in xlb
native code (stack frame is deep inside other library and this other
library was called from lisp).

On Sat, Feb 12, 2022, 09:18 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> > This native function closes the fd of the xcb and causes the select to
> > return EINTR. xcb have internal infinate loop Incase of EINTR, so closing
> > the fd is necessary to get out of this infinite loop. Closing the fd also
> > causes libx11 to realize the connection was closed and call the error
> > handler of emacs for x11 failures for a clean termination of the
> resources
> > in emacs.
>
> (I'm going through old bug reports that unfortunately weren't resolved
> at the time.)
>
> I don't think it would be appropriate to add a function like this to the
> C level of Emacs, because the use case is very limited.  And I think you
> can basically get the same functionality by using the xelb package
> (which talks to the X servers from Lisp).
>
> So I'm closing this bug report as a "wontfix".
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

[-- Attachment #2: Type: text/html, Size: 2433 bytes --]

bug#36362: New feature-x-check-server

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

otadmor <otadmor@gmail.com> writes:

> When running xlb function the code gets stuck in a native endless
> loop. The patch I have added closes the fd of the xserver, which as a
> side effect ends the endless loop.  Some would say this patch is
> fixing a bug of a dependency of emacs and not emacs itself (it is just
> that emacs uses it in a certain way...).  This solution uses native
> timer (using signals) to detect the timeout. Upon reaching a timeout
> it closes an the fd on the same thread as the xlb code (this is
> because of how signals works).

I am not happy with your change.  Firstly, it is not portable and makes
very specific assumptions about the internals of Xlib, while we support
any Xlib from X11R6 in the past to the foreseeable future, along with
some alternative implementations.  We do not use any part of Xlib that
forms part of the interface for protocol extensions, since Emacs is not
an X11 protocol extension.

Secondly, Emacs supports connecting multiple X displays at the same
time.  Your code does not try to support that at all.

Thirdly, I cannot understand what is returning EINTR: that error occurs
only when the read from the X connection was interrupted by a signal.

If the X server goes down (such as when your laptop goes to sleep), the
connection eventually times out and closes, which then triggers an IO
error that Emacs does handle correctly.  If it does not, then that is a
bug in xcb and should be reported to their developers.

I suggest to add an entry to etc/PROBLEMS describing your specific
use case instead.

> To do this is lisp we need to answer the following:
> 1. How to find the fd of the current xserver using lisp?

That is not possible, and I wouldn't agree to such a function.

> 4. Is it even allowed to run lisp code while the main thread is in xlb
> native code (stack frame is deep inside other library and this other
> library was called from lisp).

Yes, but not inside a signal handler (unless it is an IO signal, see
block_input and friends.)

bug#36362: New feature-x-check-server

[otadmor]

[-- Attachment #1: Type: text/plain, Size: 2810 bytes --]

As for your firat comment - this patch adds a lisp function to check if the
xserver is responsive. It does not run automatically for everyone, so only
those who are interested should call this function from their lisp
configuration. Personally I added run-at-time to call the exported function
from the patch.
The patch was design to not change existing behaviour of emacs. If you see
somewhere this is not the case I could fix that.
I can add ifdef to make this code compile only with this specific xlib.
Would that be ok in your opinion?
Secondly, the exported function gets the frame you wish to check.
Thirdly, this what I saw when I debugged it.


On Sat, Feb 12, 2022, 10:56 Po Lu <luangruo@yahoo.com> wrote:

> otadmor <otadmor@gmail.com> writes:
>
> > When running xlb function the code gets stuck in a native endless
> > loop. The patch I have added closes the fd of the xserver, which as a
> > side effect ends the endless loop.  Some would say this patch is
> > fixing a bug of a dependency of emacs and not emacs itself (it is just
> > that emacs uses it in a certain way...).  This solution uses native
> > timer (using signals) to detect the timeout. Upon reaching a timeout
> > it closes an the fd on the same thread as the xlb code (this is
> > because of how signals works).
>
> I am not happy with your change.  Firstly, it is not portable and makes
> very specific assumptions about the internals of Xlib, while we support
> any Xlib from X11R6 in the past to the foreseeable future, along with
> some alternative implementations.  We do not use any part of Xlib that
> forms part of the interface for protocol extensions, since Emacs is not
> an X11 protocol extension.
>
> Secondly, Emacs supports connecting multiple X displays at the same
> time.  Your code does not try to support that at all.
>
> Thirdly, I cannot understand what is returning EINTR: that error occurs
> only when the read from the X connection was interrupted by a signal.
>
> If the X server goes down (such as when your laptop goes to sleep), the
> connection eventually times out and closes, which then triggers an IO
> error that Emacs does handle correctly.  If it does not, then that is a
> bug in xcb and should be reported to their developers.
>
> I suggest to add an entry to etc/PROBLEMS describing your specific
> use case instead.
>
> > To do this is lisp we need to answer the following:
> > 1. How to find the fd of the current xserver using lisp?
>
> That is not possible, and I wouldn't agree to such a function.
>
> > 4. Is it even allowed to run lisp code while the main thread is in xlb
> > native code (stack frame is deep inside other library and this other
> > library was called from lisp).
>
> Yes, but not inside a signal handler (unless it is an IO signal, see
> block_input and friends.)
>

[-- Attachment #2: Type: text/html, Size: 3535 bytes --]

bug#36362: New feature-x-check-server

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

otadmor <otadmor@gmail.com> writes:

> As for your firat comment - this patch adds a lisp function to check
> if the xserver is responsive. It does not run automatically for
> everyone, so only those who are interested should call this function
> from their lisp configuration. Personally I added run-at-time to call
> the exported function from the patch.

No, it's only to solve a niche problem and will lead to constant
maintenance hassle with updates to Xlib.

> The patch was design to not change existing behaviour of emacs. If you
> see somewhere this is not the case I could fix that.  I can add ifdef
> to make this code compile only with this specific xlib. Would that be
> ok in your opinion?

No, see above.

> Thirdly, this what I saw when I debugged it.

Then the solution is to report the bug to the developers of whatever
component is causing the bug, and then to ack so we can describe it in
PROBLEMS with whatever workaround they might recommend.  Of course, you
are free to keep using your change yourself.

Thanks in advance.

bug#36362: The elisp timer

[otadmor .]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

This is the code of the elisp timer. This is actually a bug fix.

(defun check-frames-connection-timer ()
(safe-check-frames-connection)
(run-at-time 2 nil 'check-frames-connection-timer))
(run-at-time 2 nil 'check-frames-connection-timer)
(defun safe-x-check-frame (frame)
(condition-case nil
(x-check-frame frame)
(error nil)))
(defun safe-check-frames-connection ()
(when (functionp 'x-check-frame)
(dolist (frame (frame-list))
(when (eq (framep frame) 'x)
(safe-x-check-frame frame)))))

[-- Attachment #2: Type: text/html, Size: 9557 bytes --]

bug#36362: Tag this report as bug

[otadmor .]

[-- Attachment #1: Type: text/plain, Size: 83 bytes --]

Hey,
How can this be escalated to be a bug and be placed in the bug list?

Thanks.

[-- Attachment #2: Type: text/html, Size: 172 bytes --]

bug#36362: Tag this report as bug

[Eli Zaretskii]

> From: "otadmor ." <otadmor@gmail.com>
> Date: Tue, 30 Jul 2019 22:54:29 +0300
> 
> How can this be escalated to be a bug

A request for a new feature cannot be a bug.

> and be placed in the bug list?

It's already there.

bug#36362: Tag this report as bug

[otadmor .]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

This so called feature fixes the DOS bug I have described on previous
messages. It can be easily reproduced by the steps I wrote there.

On Wed, Jul 31, 2019, 05:27 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: "otadmor ." <otadmor@gmail.com>
> > Date: Tue, 30 Jul 2019 22:54:29 +0300
> >
> > How can this be escalated to be a bug
>
> A request for a new feature cannot be a bug.
>
> > and be placed in the bug list?
>
> It's already there.
>

[-- Attachment #2: Type: text/html, Size: 837 bytes --]

bug#36362: Tag this report as bug

[Noam Postavsky]

"otadmor ." <otadmor@gmail.com> writes:

> This so called feature fixes the DOS bug I have described on previous
> messages. It can be easily reproduced by the steps I wrote there.

The line between bug and feature request can sometimes be fuzzy (in this
case, you framed the solution as a new feature so it looks like a
feature request).  But there's no such thing as "escalation" here
anyway, we don't have extra people waiting to be assigned to solving
non-wishlist bugs.

You might want to ask on emacs-devel, maybe someone there will
understand the problem better and have some useful feedback about it.

bug#36362: Tag this report as bug

[otadmor .]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

How can I forward this to emacs devel?

On Wed, Jul 31, 2019, 14:50 Noam Postavsky <npostavs@gmail.com> wrote:

> "otadmor ." <otadmor@gmail.com> writes:
>
> > This so called feature fixes the DOS bug I have described on previous
> > messages. It can be easily reproduced by the steps I wrote there.
>
> The line between bug and feature request can sometimes be fuzzy (in this
> case, you framed the solution as a new feature so it looks like a
> feature request).  But there's no such thing as "escalation" here
> anyway, we don't have extra people waiting to be assigned to solving
> non-wishlist bugs.
>
> You might want to ask on emacs-devel, maybe someone there will
> understand the problem better and have some useful feedback about it.
>

[-- Attachment #2: Type: text/html, Size: 1159 bytes --]

bug#36362: Tag this report as bug

[Noam Postavsky]

On Wed, 31 Jul 2019 at 13:01, otadmor . <otadmor@gmail.com> wrote:
>
> How can I forward this to emacs devel?

Just send a new message to emacs-devel@gnu.org.

You can put a link to https://debbugs.gnu.org/36362 in the message so
people can read it for more background info.