feat: async #82
feat: async #82
Conversation
|
@wbolster what are your thoughts on this? |
There was a problem hiding this comment.
Hi @hraban! I have no connection to this project (besides using it), but I figured I'd review this PR since I was stuck waiting for direnv to build my project's Guix environment. 🙃
I've made some comments on specific parts of the change, but more generally I think I feel weird about this because the environment update has global effects (changing exec-path and process-environment). Making it async makes the changes feel more unpredictable - it's harder to know which environment you're in, without doing something like counting "how many times have I switched buffers, and how many environment update messages have I seen?"
This might be improved by adding something to the mode line to indicate that a direnv command is still running. If we switch to using a queue (instead of a timer) then we could also say how many direnv commands are queued up.
This issue could also be mitigated somewhat by making exec-path and process-environment buffer local, but that feels like a more invasive change that goes beyond just "making it async". (And even if you did this, showing that direnv is running in the background would still be useful.)
| (run-with-timer 1 nil 'direnv--export directory callback t)) | ||
| (when retry | ||
| (message "direnv: retrying %s" directory)) | ||
| ;; Create this asap to avoid a race condition |
There was a problem hiding this comment.
Emacs Lisp is single-threaded, and there's no yield point here, so I don't think there's any race condition here.
| ;; only one instance of direnv runs at a time. This is suboptimal: it | ||
| ;; would be nice to have parallel direnvs for every separate directory, | ||
| ;; but that would require normalising which direnv roots for buffers in | ||
| ;; different subdirectories of the same project, which can get tricky, |
There was a problem hiding this comment.
This might be easier than it seems. Setting EDITOR to echo lets us use direnv edit to find the right directory. As an example:
(defun direnv--find-root (directory)
"Find the directory containing the .envrc file that direnv will load."
(with-temp-buffer
(let* ((default-directory directory)
(process-environment (cons "EDITOR=echo" process-environment))
(exit-code (call-process direnv--executable nil t nil "edit")))
(if (zerop exit-code)
(file-name-directory (buffer-string))
(display-warning 'direnv (format-message "Error running direnv (exit code %d):\n%s" exit-code (buffer-string)))
nil))))
| (progn | ||
| (message "direnv: already running, retrying %s in 1 second (see %s)" | ||
| directory direnv--output-buffer-name) | ||
| (run-with-timer 1 nil 'direnv--export directory callback t)) |
There was a problem hiding this comment.
If we store these in a queue then we can manage these retries a bit more intelligently. If we also find the root directory before running things then we could even queue up just the callbacks (so a single long-running call to direnv returns once and updates all the relevant buffers at the same time).
There was a problem hiding this comment.
There was a problem hiding this comment.
No, sorry, I just mean a list of "things to do next". In this case, it could just be a list of (directory . callback) pairs.
The current code is using timers to poll for when they are able to run. This is nondeterministic (i.e. the tasks may run out of order), and uses cycles polling. These are unnecessary downsides, though, because you know when you're finished because the on-success callback below runs.
Instead, you could (push (list directory callback) direnv--export-queue) instead of setting a timer, then (apply #'direnv--export (pop direnv--export-queue)) at the end of on-success, on-error, and the error handler in condition-case.
| (setq exec-path (append (parse-colon-path value) (list exec-directory))) | ||
| ;; Prevent `eshell-path-env` getting out-of-sync with $PATH: | ||
| ;; eshell-path-env is deprecated in newer versions of Emacs | ||
| (when (derived-mode-p 'eshell-mode) |
There was a problem hiding this comment.
This is sensitive to the buffer that it runs in. I don't see any code to manage which buffer the callback runs in, so I expect this callback is getting run in the *direnv stdout* buffer. It works out fine for the setenv and exec-path bits (as long as process-environment and exec-path haven't been made buffer local there), but this eshell path maintenance won't work.
| (when (and direnv-always-show-summary (not (string-empty-p summary))) | ||
| (setq show-summary t)) | ||
| (when show-summary | ||
| (direnv--show-summary summary old-directory direnv--active-directory)) |
There was a problem hiding this comment.
The timing of this has changed substantially. Does this need to be passed as an argument to guard against the value changing? Or do we not really care?
|
Thank you for the excellent review @czan . I've been using this PR locally since I wrote it and while I agree with your points, it's been working well enough for me that I can't bring myself to clean this up right now. If the PR has a hope of ever getting merged I might have another look at it, but since the maintainer seems completely silent on this PR I would rather not waste my time cleaning it up further if it won't ever be used by anyone but me. But definitely good points , thanks again. |
|
No worries. I figured I'd do a review to make it easier for a maintainer to take a look and merge it. I totally understand not wanting to spend more time on it if you've solved your problem. 🙂 If I get more time in the coming weeks I might have a go at making the changes that I suggested. |
|
@czan ever since your review I can't unsee all the points you raised and my distaste with my fork and workflow has increased considerably 😂 I hope we can quickly move past this crappy solution. |
don't block the editor while loading direnv
Particularly useful for big nix flake / shell direnvs which can pull a lot of data on first init.