Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement async_worker_eval to affect environment inside worker #29

Merged
merged 1 commit into from
Aug 15, 2018

Conversation

mafredri
Copy link
Owner

@mafredri mafredri commented Aug 7, 2018

Based on #27 by @annacrombie.

Closes #25.
Closes #27.

@maximbaz
Copy link

maximbaz commented Aug 8, 2018

This looks like a good approach 👍

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

Closes #25
Closes #27

@mafredri
Copy link
Owner Author

Haha, thanks @maximbaz 😄.

@annacrombie
Copy link

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
Copy link
Owner Author

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!

@mafredri mafredri merged commit 9f8686d into master Aug 15, 2018
@annacrombie
Copy link

@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
Copy link
Owner Author

@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 60fa536 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.

@mafredri mafredri deleted the worker-eval branch August 19, 2018 19:09
@annacrombie
Copy link

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
Copy link
Owner Author

@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 &

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants