jwiegley / emacs-async

Simple library for asynchronous processing in Emacs
GNU General Public License v3.0
829 stars 68 forks source link

Fix when async-send-over-pipe is nil #154

Closed rdparker closed 1 year ago

rdparker commented 1 year ago

While debugging a hanging async process, I discovered a problem with setting async-send-over-pipe to nil.

In this case, the async package base64 encodes the function and sends it on the command line with an invocation like:

.../Emacs -Q -l .../async.elc --batch -f async-batch-invoke "<long base64 string>"

but because async-batch-invoke leaves the encoded string on command-line-args-left rather than consuming it, this results in a

file-error ("Getting attributes" "File name too long" ...)

error.

In my use case I was setting async-send-over-pipe to nil so I could list-process and copy the Emacs command-line to a terminal without the --batch argument so I could see why the process was hanging. It worked and I will be submitting a PR to the maintainer of the other repo that had the issue, but also wanted to submit this fix.

thierryvolpiatto commented 1 year ago

Ron Parker @.***> writes:

While debugging a hanging async process, I discovered a problem with setting async-send-over-pipe to nil.

In this case, the async package base64 encodes the function and sends it on the command line with an invocation like:

.../Emacs -Q -l .../async.elc --batch -f async-batch-invoke ""

but because async-batch-invoke leaves the encoded string on command-line-args-left rather than consuming it, this results in a

file-error ("Getting attributes" "File name too long" ...)

error.

Can you provide a recipe for this?

In my use case I was setting async-send-over-pipe to nil so I could list-process and copy the Emacs command-line to a terminal without the --batch argument so I could see why the process was hanging. It worked and I will be submitting a PR to the maintainer of the other repo that had the issue, but also wanted to submit this fix.

Thanks but your patch is assigning 'args' which is a free variable.

In async-batch-invoke:
async.el:241:40: Warning: assignment to free variable ‘args’
async.el:245:42: Warning: reference to free variable ‘args’

-- Thierry

thierryvolpiatto commented 1 year ago

Is this patch working for you?

diff --git a/async.el b/async.el
index 31762cf..d77dc3f 100644
--- a/async.el
+++ b/async.el
@@ -230,19 +230,17 @@ It is intended to be used as follows:
   "Called from the child Emacs process' command line."
   ;; Make sure 'message' and 'prin1' encode stuff in UTF-8, as parent
   ;; process expects.
-  (let ((coding-system-for-write 'utf-8-auto))
+  (let ((coding-system-for-write 'utf-8-auto)
+        (args-left command-line-args-left))
+    (setq command-line-args-left nil)
     (setq async-in-child-emacs t
           debug-on-error async-debug)
-    (if debug-on-error
+    (condition-case-unless-debug err
         (prin1 (funcall
                 (async--receive-sexp (unless async-send-over-pipe
-                                       command-line-args-left))))
-      (condition-case err
-          (prin1 (funcall
-                  (async--receive-sexp (unless async-send-over-pipe
-                                         command-line-args-left))))
-        (error
-         (prin1 (list 'async-signal err)))))))
+                                       args-left))))
+      (error
+       (prin1 (list 'async-signal err))))))

 (defun async-ready (future)
   "Query a FUTURE to see if it is ready.
thierryvolpiatto commented 1 year ago

@jwiegley Can you have a look at this?

Thanks.

jwiegley commented 1 year ago

This looks good to me @thierryvolpiatto.

thierryvolpiatto commented 1 year ago

John Wiegley @.***> writes:

This looks good to me @thierryvolpiatto.

Thanks John, merged in master.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.*Message ID: @.***>

-- Thierry

rdparker commented 1 year ago

Thanks Thierry. I noticed the free variable this morning and came here to correct it. The committed patch works fine.