mafredri / zsh-async

Because your terminal should be able to perform tasks asynchronously without external tools!
MIT License
766 stars 34 forks source link

Implement async_worker_eval to affect environment inside worker #29

Closed mafredri closed 6 years ago

mafredri commented 6 years ago

Based on #27 by @annacrombie.

Closes #25. Closes #27.

maximbaz commented 6 years ago

This looks like a good approach 👍

P.S. "Closes" only works on one ID 😉 , change to:

Closes #25 Closes #27

mafredri commented 6 years ago

Haha, thanks @maximbaz 😄.

annacrombie commented 6 years ago

I can't test this at the moment, but I'd say everything looks good to me. I wanted to implement this too but was too worried about getting tripped up by strange shell expansions.

mafredri commented 6 years ago

Thanks for the feedback @maximbaz and @annacrombie.

I wanted to implement this too but was too worried about getting tripped up by strange shell expansions.

I feel ya 😄! At least this uses much of the same code path as async_job so if there's an issue it needs to be fixed in both.

I'll go ahead and release this, please report back if you run into any issues!

annacrombie commented 6 years ago

@mafredri, I finally got around to testing this, and just a little thing I noticed: The cat coproc is spawned before the eval occurs so cat does not inherit the environment, which isn't acceptable for my use case. I easily got around this by just reorganizing the code a little as you can see here. I wonder if this messes anything else up, or perhaps it is only me who would need this behaviour. Anyways I thought I would let you know.

mafredri commented 6 years ago

@annacrombie ah yes, sorry, I had forgotten about that detail with regards to your use case (pwd of deepest process).

I'm curious, using this branch, do you run into the issue easily or is it rare? Even with https://github.com/mafredri/zsh-async/commit/60fa53658ae2437c15aec99326d37c722598faef I imagine the issue should still present itself, at least with long-running async tasks.

The coproc will be running for as long as there is a async job running, meaning that even with your proposed fix, the pwd of the coproc would not be updated.

As an alternate solution, would it suffice to issue async_flush_jobs my_worker after updating the pwd? That would ensure that no jobs are running and the coproc is terminated. To me it seems like the most fool-proof way to ensure your use-case works.

If we also included your patch, then the order would not matter flush -> update vs update -> flush, same end-result. Without your patch it has to be update -> flush.

annacrombie commented 6 years ago

As soon as I updated, I was noticing this problem. With 60fa536, the issue was partially fixed, the first change of directory would be propogated but future changes were not. This was happening wether or not I did async_flush_jobs. I was able to finally fix this by just killing the coproc before any "async eval" operation took place. See here. I am not really sure why async_flush_jobs wasn't working but this isn't something I wanted to spend too much time on. I'm not really recommending that you include my patches, I'm just letting you know in case there is something interesting to you.

mafredri commented 6 years ago

@annacrombie thanks for the detailed explanation. Looking at this again, and your patches, I have a potential fix for you that I could merge in if it works (it's more correct anyway).

What I think is happening is that each eval is starting a coproc but the child trap is never triggered because we aren't actually running any forked processes. Might be wrong though because in theory, without any patches, the coproc should be killed / restarted if you've run any async jobs after changing directories.

diff --git a/async.zsh b/async.zsh
index d35f128..fb2ff81 100644
--- a/async.zsh
+++ b/async.zsh
@@ -198,6 +198,7 @@ _async_worker() {
            shift cmd  # Strip _async_eval from cmd.
            _async_eval $cmd
            do_eval=0
+           child_exit  # Manually trigger after eval (exit coproc).
        else
            # Run job in background, completed jobs are printed to stdout.
            _async_job $cmd &