bug#45821: Emacs UDP support on Windows

[Alex Matei <matei.alexandru@live.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4678 bytes --]

Here are the 2 patches that can be applied, in order:

  *   0001 -> the original patch from the email thread, that I used
  *   0002 -> the additions that I made locally

Note: TLS mostly works, but I’ve observed some weird behavior in the process list for EWW

  *   It almost feels like sometimes we hit snags in navigating to some HTTPS endpoints because the existing connections never get closes
  *   I think we need to make some investigations/ changes to the socket lifetime / close behavior to fix the class of issues we are seeing in EWW, where sometimes the page load hangs, and yes, you can load it after a couple of refreshes (‘g’ key binding) but then you see something really interesting if you take a look at the process list
     *   You will notice that some of the connection never close (properly) and hang around!
     *   If anyone has any good pointers of what are good functions to look at, please let me know; I will take a look at this later in the day, and navigate the reader thread and everything around it..

[cid:image003.png@01D91F6E.1C5C1280]

p.s. I didn’t find an obvious way of merging the two patches in magit, hence the 2 individual patches 😊 – I guess I can just rebase the commits into one..



Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows

From: Robert Pluim<mailto:rpluim@gmail.com>
Sent: Tuesday, January 3, 2023 12:51 AM
To: Alex Matei<mailto:matei.alexandru@live.com>
Cc: Eli Zaretskii<mailto:eliz@gnu.org>; 45821@debbugs.gnu.org<mailto:45821@debbugs.gnu.org>
Subject: Re: bug#45821: Emacs UDP support on Windows

>>>>> On Mon, 2 Jan 2023 22:56:04 +0000, Alex Matei <matei.alexandru@live.com> said:

    Alex> Thanks for the tips with logging!
    Alex> I think I found the fix (at least for one of the issues?)
    Alex> Please see the attached patch/diff for the fix.

    Alex> The code will always ignore (aka ‘eat’) one character if the file
    Alex> descriptor had the least significant bit set (0x1, so this would mean
    Alex> for any file descriptor with READ flag set)
    Alex> [cid:image004.png@01D91EBA.5498F140]

Hmm, my local version already had that fixed, so I guess I posted the
wrong patch to the bug. But I still think you found a fix (see below)

    Alex> Based on operator precedence rules, FILE_SOCKET == 0 will be evaluated
    Alex> first (before &) , resulting in 0, which would effectively cause the
    Alex> condition to always be false

    Alex> This behavior can be observed when executing a call to
    Alex> ‘(async-shell-command)’ (-> file descriptor flags 0x191) on a build
    Alex> with the patch vs one without the patch applied (see below, attached
    Alex> image)

    Alex>   * My suspicion is that this behavior is similar to what TLS was
    Alex> experiencing but just easier to reproduce (note: there might other
    Alex> issues with TLS , but so far, after applying this fix I have not run
    Alex> into any other issues)

    Alex> [cid:image005.png@01D91EBA.5498F140]


    Alex> @Robert Pluim<mailto:rpluim@gmail.com> I don’t know if this was the
    Alex> only missing piece, but from all my tests I couldn’t see any issues
    Alex> with TLS anymore, and with navigating https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgnu.org%2F&data=05%7C01%7C%7C4f957b24a1204b5c8e5608daed67ab41%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638083326770863958%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oe0n96z7hwrRvkyKmiiG6Bpjn21g5OUcgkeV6pklPs4%3D&reserved=0 in EWW..
    Alex> I wanted to thank you for creating this patch 🙏, and giving me pointers on how to apply and debug it.

Iʼd run the networking portion of our test suite to see if everything
works as intended (you might need to install the GnuTLS cli tools). eg:

    cd test
    make network-stream-tests

    Alex> p.s.
    Alex> - The patch also adds STATUS_READ_IN_PROGRESS state for the new
    Alex> ‘_sys_wait_readable’ function , based on what the read ahead logic was
    Alex> doing already (idk if it is needed, but it is a nice mirror, and a
    Alex> good status code to reflect, although I am not convinced anyone cares
    Alex> about this state transition..)

I think this is actually the fix, since I added it locally and TLS
started working, but I donʼt understand the code enough to be definite
about that. More testing required 😀

Can you post the full patch just to ensure weʼre all talking about the
same changes? Iʼve attached what Iʼm working from

Robert
--


[-- Attachment #1.1.2: Type: text/html, Size: 11475 bytes --]

[-- Attachment #1.2: 51F8D5BC072047BE9A0B359605491906.png --]
[-- Type: image/png, Size: 348894 bytes --]

[-- Attachment #2: 0001-28.0.50-Add-UDP-support-for-Emacs-on-Windows.patch --]
[-- Type: application/octet-stream, Size: 3927 bytes --]

From 93fc2aad5383a43abd5476cda39fb30e24176d28 Mon Sep 17 00:00:00 2001
From: Lars Ingebrigtsen <larsi at>
Date: Tue, 12 Jan 2021 19:08:12 +0100
Subject: [PATCH 1/2] 28.0.50; Add UDP support for Emacs on Windows

This patch from Robert Pluim adds that support, but needs somebody that
actually uses Windows to test it.

As it stands you need to arrange for WORKING_SELECT_EMULATION to be
defined.
---
 nt/inc/ms-w32.h |  3 ++-
 src/w32.c       | 53 +++++++++++++++++++++++++++++++++++++++++++++----
 src/w32.h       |  3 +++
 src/w32proc.c   |  7 ++++++-
 4 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/nt/inc/ms-w32.h b/nt/inc/ms-w32.h
index 3f4b2f3489..522d2d9ee5 100644
--- a/nt/inc/ms-w32.h
+++ b/nt/inc/ms-w32.h
@@ -89,10 +89,11 @@ #define _CALLBACK_ __cdecl
    Look in <sys/time.h> for a timeval structure.  */
 #define HAVE_TIMEVAL 1
 
+#ifndef WORKING_SELECT_EMULATION
 /* Our select emulation does 1-byte read-ahead waiting for received
    packets, so datagrams are broken.  */
 #define BROKEN_DATAGRAM_SOCKETS 1
-
+#endif
 #define MAIL_USE_SYSTEM_LOCK 1
 
 /* Define to 1 if GCC-style __attribute__ ((__aligned__ (expr))) works. */
diff --git a/src/w32.c b/src/w32.c
index 00b8c2515d..e11fa805f2 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -8912,6 +8912,45 @@ _sys_wait_accept (int fd)
   return cp->status;
 }
 
+#ifdef WORKING_SELECT_EMULATION
+int
+_sys_wait_readable (int fd)
+{
+  HANDLE hEv;
+  child_process * cp;
+  int rc;
+
+  if (fd < 0 || fd >= MAXDESC)
+    return STATUS_READ_ERROR;
+
+  cp = fd_info[fd].cp;
+
+  if (cp == NULL || cp->fd != fd || cp->status != STATUS_READ_READY)
+    return STATUS_READ_ERROR;
+
+  cp->status = STATUS_READ_FAILED;
+
+  hEv = pfn_WSACreateEvent ();
+  rc = pfn_WSAEventSelect (SOCK_HANDLE (fd), hEv, FD_READ);
+  if (rc != SOCKET_ERROR)
+    {
+      do
+        {
+          rc = WaitForSingleObject (hEv, 500);
+          Sleep (5);
+        } while (rc == WAIT_TIMEOUT
+                 && cp->status != STATUS_READ_ERROR
+                 && cp->char_avail);
+      pfn_WSAEventSelect (SOCK_HANDLE (fd), NULL, 0);
+      if (rc == WAIT_OBJECT_0)
+        cp->status = STATUS_READ_SUCCEEDED;
+    }
+  pfn_WSACloseEvent (hEv);
+
+  return cp->status;
+}
+#endif
+
 int
 _sys_wait_connect (int fd)
 {
@@ -9037,10 +9076,16 @@ sys_read (int fd, char * buffer, unsigned int count)
 	      return -1;
 
 	    case STATUS_READ_SUCCEEDED:
-	      /* consume read-ahead char */
-	      *buffer++ = cp->chr;
-	      count--;
-	      nchars++;
+#ifdef WORKING_SELECT_EMULATION
+              /* select on sockets no longer requires a 1-byte read.  */
+              if (fd_info[fd].flags & FILE_SOCKET == 0)
+#endif
+		{
+		  /* consume read-ahead char */
+		  *buffer++ = cp->chr;
+		  count--;
+		  nchars++;
+		}
 	      cp->status = STATUS_READ_ACKNOWLEDGED;
 	      ResetEvent (cp->char_avail);
 
diff --git a/src/w32.h b/src/w32.h
index 8a5c4ecbc7..50b03ae078 100644
--- a/src/w32.h
+++ b/src/w32.h
@@ -175,6 +175,9 @@ #define FILE_SERIAL             0x0800
 
 extern int _sys_read_ahead (int fd);
 extern int _sys_wait_accept (int fd);
+#ifdef WORKING_SELECT_EMULATION
+extern int _sys_wait_readable (int fd);
+#endif
 extern int _sys_wait_connect (int fd);
 
 extern HMODULE w32_delayed_load (Lisp_Object);
diff --git a/src/w32proc.c b/src/w32proc.c
index 3a6504c925..71337bed4d 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -1225,7 +1225,12 @@ reader_thread (void *arg)
       else if (cp->fd >= 0 && (fd_info[cp->fd].flags & FILE_LISTEN) != 0)
 	rc = _sys_wait_accept (cp->fd);
       else
-	rc = _sys_read_ahead (cp->fd);
+#ifdef WORKING_SELECT_EMULATION
+        if (fd_info[cp->fd].flags & FILE_SOCKET)
+          rc = _sys_wait_readable (cp->fd);
+        else
+#endif
+          rc = _sys_read_ahead (cp->fd);
 
       /* Don't bother waiting for the event if we already have been
 	 told to exit by delete_child.  */
-- 
2.37.2.windows.2


[-- Attachment #3: 0002-Fix-missing-characters-in-shell-and-potentially-TLS.patch --]
[-- Type: application/octet-stream, Size: 1393 bytes --]

From 17992fcf180451a6fb9508e33ad5c5573f7bdc9b Mon Sep 17 00:00:00 2001
From: Alex Matei <almat@microsoft.com>
Date: Tue, 3 Jan 2023 11:59:18 -0800
Subject: [PATCH 2/2] Fix missing characters in shell and potentially TLS

---
 src/w32.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/w32.c b/src/w32.c
index e11fa805f2..01a8616839 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -8928,7 +8928,7 @@ _sys_wait_readable (int fd)
   if (cp == NULL || cp->fd != fd || cp->status != STATUS_READ_READY)
     return STATUS_READ_ERROR;
 
-  cp->status = STATUS_READ_FAILED;
+  cp->status = STATUS_READ_IN_PROGRESS;
 
   hEv = pfn_WSACreateEvent ();
   rc = pfn_WSAEventSelect (SOCK_HANDLE (fd), hEv, FD_READ);
@@ -8944,6 +8944,8 @@ _sys_wait_readable (int fd)
       pfn_WSAEventSelect (SOCK_HANDLE (fd), NULL, 0);
       if (rc == WAIT_OBJECT_0)
         cp->status = STATUS_READ_SUCCEEDED;
+      else
+	cp->status = STATUS_READ_FAILED;
     }
   pfn_WSACloseEvent (hEv);
 
@@ -9078,7 +9080,7 @@ sys_read (int fd, char * buffer, unsigned int count)
 	    case STATUS_READ_SUCCEEDED:
 #ifdef WORKING_SELECT_EMULATION
               /* select on sockets no longer requires a 1-byte read.  */
-              if (fd_info[fd].flags & FILE_SOCKET == 0)
+              if ((fd_info[fd].flags & FILE_SOCKET) == 0)
 #endif
 		{
 		  /* consume read-ahead char */
-- 
2.37.2.windows.2