skeeto / emacs-aio

async/await for Emacs Lisp
The Unlicense
218 stars 12 forks source link

Usage with shell command #19

Open rougier opened 3 years ago

rougier commented 3 years ago

I'm trying to use aio to run a shell command but my call seems to be blocking (cannot move cursor until stop is written). I imagine I'm doing something wrong but I'm not quite sure where:

(defun aio-shell (command)
  (let ((promise (aio-promise)))
    (prog1 promise
      (aio-resolve promise (lambda () (shell-command-to-string command))))))

(aio-defun aio-run (command)
  (interactive)
  (message "Start") (aio-await (aio-shell command)) (message "Stop"))

(aio-run "sleep 3")

Is that the right syntax?

alphapapa commented 3 years ago

I'm guessing that won't work because shell-command-to-string boils down to call-process which is synchronous. Then again, I'm far from an expert here. Anyway, I'd suggest building a process-calling function that's async all the way down.

rougier commented 3 years ago

Thanks. Would you have a pointer to a simple example that I can take inspiration from?

Phundrak commented 3 years ago

From this email in the Emacs mailing list, I found something that could be useful. I rewrote it the following way in order to be sure no conflict would happen between several launches of async-shell-command-to-string.

(defun async-shell-command-to-string (command callback)
  (with-temp-buffer
    (lexical-let ((output-buffer (current-buffer))
                  (callback-fun callback))
      (set-process-sentinel (shell-command command output-buffer)
                            (lambda (process signal)
                              (when (memq (process-status process) '(exit signal))
                                (with-current-buffer output-buffer
                                  (funcall callback-fun (buffer-string)))
                                (kill-buffer output-buffer))))
      output-buffer)))

And it works like a charm, with this as a quick but simple test:

(progn
  (async-shell-command-to-string "sleep 3; echo World"
                                 (lambda (output) (message output)))
  (async-shell-command-to-string "echo Hello"
                                 (lambda (output) (message output))))

I would love to see an equivalent that would actually return the string and not the temporary buffer once the shell command is executed instead on relying on a callback function though. I also don’t know if getting rid of buffer-substring-no-properties from the original code was a good idea or not.

alphapapa commented 3 years ago

Hm, well:

  1. That doesn't use aio anymore. :)
  2. You shouldn't use lexical-let but rather set lexical-binding in the buffer.
Phundrak commented 3 years ago
  1. That doesn't use aio anymore. :)

The core issue seemed to be with shell-command-to-string not being async. Now maybe we can wrap this definition of async-shell-command-to-string in an aio function that would be able to make use of the value passed in the callback function? Right now, I don’t know how though.

  1. You shouldn't use lexical-let but rather set lexical-binding in the buffer.

True, I kept it from the original implementation since I was testing things in the *scratch* buffer, it should be a simple let instead. Thanks for noticing that!

rougier commented 3 years ago

@Phundrak Thanks! I tested if on emacs 27.1 by evaluating your code in the scratch buffer but it is blocking and after 3 seconds I got an error let: Wrong type argument: processp, 1 (note that I replaced lexical-let with let and I added lexical-binding).

sam217pa commented 3 years ago

Hi, @rougier maybe you could use async-shell-command and then populate a string with the output buffer once the promise is resolved?

rougier commented 3 years ago

Yep, just found this answer on stack overflow.

skeeto commented 3 years ago

You must call aio-resolve in a callback, and the function you pass to aio-resolve must simply return the result (or signal an error), not have any side effects since it's potentially called more than once. If the action you want to run asynchronously doesn't support a callback, then it's probably not asynchronous, which is the case here.

Here's one way to write this:

(defun aio-call-process (program buffer &rest args)
  (let ((process (apply #'start-process program buffer program args))
        (promise (aio-promise)))
    (prog1 promise
      (setf (process-sentinel process)
            (lambda (_ status) (aio-resolve promise (lambda () status)))))))

An asynchronous function can aio-await this to receive the sentinel status directly, without any explicit callbacks and without blocking Emacs.

rougier commented 3 years ago

Thanks! Based on your code, I tried to adapt my below without much success:

(defun aio-call-process (program buffer &rest args)
  (let ((process (apply #'start-process program buffer program args))
        (promise (aio-promise)))
    (prog1 promise
      (setf (process-sentinel process)
            (lambda (_ status) (aio-resolve promise (lambda () status)))))))

(aio-defun aio-run (command &rest args)
  (interactive)
  (message "Start")
  (aio-await (aio-call-process command (current-buffer) args))
  (message "Stop"))

(aio-run "sleep" 3)
alphapapa commented 3 years ago

Thanks! Based on your code, I tried to adapt my below without much success:

What exactly happens?

rougier commented 3 years ago

This is the log in *Messages* (that I get without any delay):

Start
#s(aio-promise (closure ((cps-state-atom-100 closure #2 nil ...) (cps-state-atom-99 closure #2 nil ...) (cps-state-iter-yield-98 closure #2 nil ...) (cps-state-let*-97 closure #2 nil ...) (cps-state-atom-96 closure #2 nil ...) (cps-binding-cps-argument-94-95) (cps-state-atom-93 closure #2 nil ...) (cps-state-let*-92 closure #2 nil ...) (cps-state-atom-91 closure #2 nil ...) (cps-binding-cps-binding-result-89-90) (cps-binding-result-89) (cps-state-atom-88 closure #2 nil ...) ...) nil (signal (car cps-binding-condition-case-error-86) (cdr cps-binding-condition-case-error-86))) nil)
alphapapa commented 3 years ago

What are you expecting to happen?

rougier commented 3 years ago

Start in the log, then non blocking pause of 3 second and then Stop in the log. Did I miss something obvious ? In the meantime, I managed to implement what I wanted but the code is ugly

alphapapa commented 3 years ago

Start in the log, then non blocking pause of 3 second and then Stop in the log. Did I miss something obvious ?

The output you showed doesn't indicate whether there was a non-blocking, 3-second pause, and it doesn't say Stop at all. So I didn't understand what was going on. I'm still not sure that I do understand, for that reason. :)

In the meantime, I managed to implement what I wanted but the code is ugly

That code looks nice to me. :shrug:

rougier commented 3 years ago

Oh wait, I'm totally wrong here. Since the call is non-blocking, I should have start/stop immediately in the log and then a background sleep without any output.

skeeto commented 3 years ago

There are three issues here. First you're not seeing any errors since you're running the code asynchronously, and there's nobody to receive the error signal. When you're debugging use aio-wait-for at the top-level, which is like aio-await but blocks the thread until the promise is resolved, receiving any errors.

My aio-call-process doesn't take a list of arguments, so if you want to pass a list you must use apply. Using aio-wait-for reveals this:

(aio-wait-for (aio-run "sleep" 3))
;; error: Wrong type argument: stringp, (3)

Correcting for this:

(aio-defun aio-run (command &rest args)
  (interactive)
  (message "Start")
  (aio-await (apply #'aio-call-process command (current-buffer) args))
  (message "Stop"))

Though there's still another issue: process arguments must be strings:

(aio-wait-for (aio-run "sleep" 3))
;; error: Wrong type argument: stringp, 3

So finally this achieves the desired effect, and runs it all in the background:

(aio-run "sleep" "3")
alphapapa commented 3 years ago

Thanks, Chris, even though this isn't my code, your explanations are very helpful for my understanding of how aio works.

Do you think this aio-call-process function would be a good addition to the library?

rougier commented 3 years ago

Thanks for you explanation, now it makes sense. I support the idea of having an aio-call-process.

skeeto commented 3 years ago

would be a good addition to the library?

Yeah, it would! Revisiting this now, I was surprised I hadn't added one before. I think my intention was for users to build their own when they need it using aio-make-callback rather than add wrappers around Emacs' many process creation functions, forwarding all their options through a new aio-based API.

Note that my example aio-call-process is incomplete. Resolution should happen when the status is "finished\n" (yeah, I hate this Emacs API), not just when the sentinel is called.

justinbarclay commented 1 year ago

I think I like the idea of having a simple aio-call-process to solve the easy cases and then encourage aio-make-callback for the more complex cases. Though, I am still curious how aio-make-callback works or improves interacting with processes or other callback-based tasks.