bug#73199: soap-client; soap-invoke-internal is not thread-safe

bug#73199: soap-client; soap-invoke-internal is not thread-safe

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

Forward to debbugs

From: Michael Albinus <michael.albinus@gmx.de>
Subject: soap-client; soap-invoke-internal is not thread-safe
To: Alexandru Harsanyi <AlexHarsanyi@gmail.com>
Date: Thu, 12 Sep 2024 15:57:45 +0200 (19 minutes, 24 seconds ago)

In package debbugs, I try to use Emacs Lisp threads when retrieving SOAP
data from the debbugs server. However, ocaasionally I run into the error

--8<---------------cut here---------------start------------->8---
(error "Attempt to accept output from process debbugs.gnu.org locked to thread #<thread 0xc288a0>")
--8<---------------cut here---------------end--------------->8---

My work around is to advice url-http-create-request

--8<---------------cut here---------------start------------->8---
  (advice-add
   'url-http-create-request :around
   (lambda (orig-fun)
     "Set `url-http-attempt-keepalives' to nil."
     (setq url-http-attempt-keepalives nil)
     (funcall orig-fun))
   '(name debbugs-advice))
--8<---------------cut here---------------end--------------->8---

However, it would be great if soap-invoke-internal could care.

Emacs  : GNU Emacs 31.0.50 (build 35, x86_64-pc-linux-gnu, GTK+ Version 3.24.43, cairo version 1.18.0)
 of 2024-09-12
Package: soap-client

bug#73199: soap-client; soap-invoke-internal is not thread-safe

[Thomas Fitzsimmons]

Hi Michael,

Michael Albinus writes:

> Forward to debbugs
>
> From: Michael Albinus <michael.albinus@gmx.de>
> Subject: soap-client; soap-invoke-internal is not thread-safe
> To: Alexandru Harsanyi <AlexHarsanyi@gmail.com>
> Date: Thu, 12 Sep 2024 15:57:45 +0200 (19 minutes, 24 seconds ago)
>
> In package debbugs, I try to use Emacs Lisp threads when retrieving SOAP
> data from the debbugs server. However, ocaasionally I run into the error
>
> --8<---------------cut here---------------start------------->8---
> (error "Attempt to accept output from process debbugs.gnu.org locked to thread #<thread 0xc288a0>")
> --8<---------------cut here---------------end--------------->8---
>
> My work around is to advice url-http-create-request
>
> --8<---------------cut here---------------start------------->8---
>   (advice-add
>    'url-http-create-request :around
>    (lambda (orig-fun)
>      "Set `url-http-attempt-keepalives' to nil."
>      (setq url-http-attempt-keepalives nil)
>      (funcall orig-fun))
>    '(name debbugs-advice))
> --8<---------------cut here---------------end--------------->8---
>
> However, it would be great if soap-invoke-internal could care.

I am not able to test Excorporate's usage of soap-client anymore to
check if a candidate patch would break anything.  I will ask on
emacs-devel if anyone wants to take over maintenance of Excorporate and
its dependencies.

A starting point for adding thread safety would be a minimal test case
that calls soap-invoke-internal against the debbugs server from multiple
different threads.  Are you able to extract such a test case?

If the resulting patch is obviously safe for single-threaded usage by
Excorporate, then I could push it to soap-client.el.

Thanks,
Thomas

bug#73199: soap-client; soap-invoke-internal is not thread-safe

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

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

> Hi Michael,

Hi Thomas,

sorry for the late reply, I was out of order for a week.

> A starting point for adding thread safety would be a minimal test case
> that calls soap-invoke-internal against the debbugs server from multiple
> different threads.  Are you able to extract such a test case?

Install the just-released debbugs 0.41. Open a new Emacs session, load
debbugs-gnu.

Deactivate the advice I have added to mitigate the problem.

--8<---------------cut here---------------start------------->8---
(defalias 'debbugs-compat-add-debbugs-advice 'ignore)
(defalias 'debbugs-compat-remove-debbugs-advice 'ignore)
--8<---------------cut here---------------end--------------->8---

Retrieve the most recent 10 bugs. This happens in the main thread.

--8<---------------cut here---------------start------------->8---
M-x debbugs-gnu-bugs RET RET
--8<---------------cut here---------------end--------------->8---

Retrieve the most recent 100 bugs. This happens in another thread.

--8<---------------cut here---------------start------------->8---
M-x debbugs-gnu-bugs RET -100 RET
--8<---------------cut here---------------end--------------->8---

You'll see the error.

--8<---------------cut here---------------start------------->8---
Error #<thread debbugs>: (error "Attempt to accept output from process debbugs.gnu.org locked to thread #<thread 0xc288a0>")
--8<---------------cut here---------------end--------------->8---

BTW, in a short discussion on emacs-devel, Stefan Monnier proposed to
fix this problem in url-http.el. Don't know what's the better approach.

> Thanks,
> Thomas

Best regards, Michael.

bug#73199: url-retrieve is not thread-safe

[Eli Zaretskii]

> Cc: AlexHarsanyi@gmail.com, Thomas Fitzsimmons <fitzsim@fitzsim.org>,
>  monnier@iro.umontreal.ca
> Date: Mon, 30 Sep 2024 17:12:34 +0200
> From:  Michael Albinus via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > BTW, in a short discussion on emacs-devel, Stefan Monnier proposed to
> > fix this problem in url-http.el. Don't know what's the better approach.
> 
> I gave it a try. The appended patch seems to fix the problem. At least
> in my use case, there's no error anymore when I use debbugs w/o the
> advice in debbugs-compat.el.
> 
> Comments?

ENOPATCH

bug#73199: url-retrieve is not thread-safe

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

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Comments?
>
> ENOPATCH

Oops, sorry. I've appended the vc-dir buffer, not the vc-diff ...


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 2890 bytes --]

diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
index 184c1278072..019ba2d7e57 100644
--- a/lisp/url/url-http.el
+++ b/lisp/url/url-http.el
@@ -74,7 +74,9 @@ url-http-proxy-basic-auth-storage

 (defvar url-http-open-connections (make-hash-table :test 'equal
 						   :size 17)
-  "A hash table of all open network connections.")
+  "A hash table of all open network connections.
+If Emacs is compiled with thread support, the key is a list `(host port
+thread)'.  Otherwise, it is a cons cell `(host . port)'.")

 (defvar url-http-version "1.1"
   "What version of HTTP we advertise, as a string.
@@ -154,26 +156,33 @@ url-http-debug
   (apply #'url-debug 'http args))

 (defun url-http-mark-connection-as-busy (host port proc)
-  (url-http-debug "Marking connection as busy: %s:%d %S" host port proc)
-  (set-process-query-on-exit-flag proc t)
-  (puthash (cons host port)
-	      (delq proc (gethash (cons host port) url-http-open-connections))
-	      url-http-open-connections)
-  proc)
+  (let ((key (if main-thread
+                 (list host port (funcall 'current-thread)) (cons host port))))
+    (url-http-debug "Marking connection as busy: %s:%d %S" host port proc)
+    (set-process-query-on-exit-flag proc t)
+    (puthash key
+             (delq proc (gethash key url-http-open-connections))
+	     url-http-open-connections)
+    proc))

 (defun url-http-mark-connection-as-free (host port proc)
-  (url-http-debug "Marking connection as free: %s:%d %S" host port proc)
-  (when (memq (process-status proc) '(open run connect))
-    (set-process-buffer proc nil)
-    (set-process-sentinel proc 'url-http-idle-sentinel)
-    (set-process-query-on-exit-flag proc nil)
-    (puthash (cons host port)
-	     (cons proc (gethash (cons host port) url-http-open-connections))
-	     url-http-open-connections))
-  nil)
+  (let ((key (if main-thread
+                 (list host port (funcall 'current-thread)) (cons host port))))
+    (url-http-debug "Marking connection as free: %s:%d %S" host port proc)
+    (when (memq (process-status proc) '(open run connect))
+      (set-process-buffer proc nil)
+      (set-process-sentinel proc 'url-http-idle-sentinel)
+      (set-process-query-on-exit-flag proc nil)
+      (puthash key
+	       (cons proc (gethash key url-http-open-connections))
+	       url-http-open-connections))
+    nil))

 (defun url-http-find-free-connection (host port &optional gateway-method)
-  (let ((conns (gethash (cons host port) url-http-open-connections))
+  (let ((conns (gethash
+                (if main-thread
+                    (list host port (funcall 'current-thread)) (cons host port))
+                url-http-open-connections))
 	(connection nil))
     (while (and conns (not connection))
       (if (not (memq (process-status (car conns)) '(run open connect)))

bug#73199: url-retrieve is not thread-safe

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 73199@debbugs.gnu.org,  AlexHarsanyi@gmail.com,  fitzsim@fitzsim.org,
>   monnier@iro.umontreal.ca
> Date: Mon, 30 Sep 2024 18:42:55 +0200
> 
> Oops, sorry. I've appended the vc-dir buffer, not the vc-diff ...

Thanks.  I'd appreciate if you could explain why url-retrieve is
currently not thread-safe, and how this patch is supposed to solve
that.

bug#73199: url-retrieve is not thread-safe

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Albinus <michael.albinus@gmx.de>
>> Cc: 73199@debbugs.gnu.org,  AlexHarsanyi@gmail.com,  fitzsim@fitzsim.org,
>>   monnier@iro.umontreal.ca
>> Date: Mon, 30 Sep 2024 18:42:55 +0200
>>
>> Oops, sorry. I've appended the vc-dir buffer, not the vc-diff ...
>
> Thanks.  I'd appreciate if you could explain why url-retrieve is
> currently not thread-safe, and how this patch is supposed to solve
> that.

In <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73199#14>, there's a
recipe for reproduction.

The general problem is when you want to access the same HTTP server from
a different thread, and url-http-attempt-keepalives is set to t. This
happens, for example, in debbugs 0.41.

Let's assume, there is an access to debbugs:443 from a given thread, for
example the main thread. Soap-client serves. It also binds
url-http-attempt-keepalives to t, the https process stays alive.

Later on, there is an access to debbugs:443 from another thread. It
checks, whether there is a running process for that target, and tries to
reuse. We get the error

--8<---------------cut here---------------start------------->8---
Error #<thread debbugs>: (error "Attempt to accept output from process debbugs.gnu.org locked to thread #<thread 0xc288a0>")
--8<---------------cut here---------------end--------------->8---

My patch tries to fix this. The list of still open processes is the hash
table url-http-open-connections. It has the ky (cons host port), and as
value a list of processes.

My patch changes this such a way, that the key is now (list host port
thread). Therefore, only open processes which belong to the same thread
are checked.

This seems to work sufficiently.

Best regards, Michael.

bug#73199: url-retrieve is not thread-safe

[Eli Zaretskii]

bug#73199: url-retrieve is not thread-safe

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

Michael Albinus <michael.albinus@gmx.de> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Michael Albinus <michael.albinus@gmx.de>
>>> Cc: 73199@debbugs.gnu.org,  AlexHarsanyi@gmail.com,  fitzsim@fitzsim.org,
>>>   monnier@iro.umontreal.ca
>>> Date: Mon, 30 Sep 2024 18:42:55 +0200
>>>
>>> Oops, sorry. I've appended the vc-dir buffer, not the vc-diff ...
>>
>> Thanks.  I'd appreciate if you could explain why url-retrieve is
>> currently not thread-safe, and how this patch is supposed to solve
>> that.
>
> In <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73199#14>, there's a
> recipe for reproduction.
>
> The general problem is when you want to access the same HTTP server from
> a different thread, and url-http-attempt-keepalives is set to t. This
> happens, for example, in debbugs 0.41.
>
> Let's assume, there is an access to debbugs:443 from a given thread, for
> example the main thread. Soap-client serves. It also binds
> url-http-attempt-keepalives to t, the https process stays alive.
>
> Later on, there is an access to debbugs:443 from another thread. It
> checks, whether there is a running process for that target, and tries to
> reuse. We get the error
>
> Error #<thread debbugs>: (error "Attempt to accept output from process debbugs.gnu.org locked to thread #<thread 0xc288a0>")
>
> My patch tries to fix this. The list of still open processes is the hash
> table url-http-open-connections. It has the ky (cons host port), and as
> value a list of processes.
>
> My patch changes this such a way, that the key is now (list host port
> thread). Therefore, only open processes which belong to the same thread
> are checked.
>
> This seems to work sufficiently.

Ping. Any comments? Otherwise, I'll install the patch in a couple of days.

Best regards, Michael.

bug#73199: url-retrieve is not thread-safe

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: AlexHarsanyi@gmail.com,  fitzsim@fitzsim.org,  monnier@iro.umontreal.ca,
>   73199@debbugs.gnu.org
> Date: Thu, 10 Oct 2024 13:29:42 +0200
> 
> Ping. Any comments? Otherwise, I'll install the patch in a couple of days.

Sorry, too much stuff on my table.

Please go ahead and install, I think the patch is okay.  (I presume
you know about set-process-thread, and that it doesn't help in this
scenario?)

bug#73199: url-retrieve is not thread-safe

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

Version: 31.1

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> Please go ahead and install, I think the patch is okay.

Done. I'm closing the bug, therefore.

> (I presume you know about set-process-thread, and that it doesn't help
> in this scenario?)

Yes, I know it. But I cannot use it; the process is already in the
proper thread where it should be.

Best regards, Michael.