-
Notifications
You must be signed in to change notification settings - Fork 35
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
prevent infinite loop in case of broken zpty file descriptor #31
Conversation
The ci build failure seems spurious to me. Please tell me if there is something to fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is great! We could handle this in Pure as well to restart the worker automatically if this error is returned. I've mostly just seen zpty crash on old versions of zsh but it has happened one or two other times as well, so this is a welcome addition 😄.
Don't worry about the travis failure. It's only zsh 5.0.8 and 5.0.2 that fail, and that's because of e6d9372. There's no .xz
version available for those and I haven't bothered updating the script to use the different source for 5.0.8/5.0.2 :/.
async.zsh
Outdated
if [[ -n $callback ]]; then | ||
$callback '[async]' 2 "" 0 "$callback:zle -F $1 returned error $2" 0 | ||
fi | ||
return 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does returning 1 have any effect here? If not, I think an empty return
would be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
async.zsh
Outdated
async_stop_worker $worker | ||
|
||
if [[ -n $callback ]]; then | ||
$callback '[async]' 2 "" 0 "$callback:zle -F $1 returned error $2" 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think mentioning the $worker
in the message would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right. Mentioning the worker makes more sense than the callback since it is the callback which receives the error message. Fixed.
In some cases, the fd created by zpty can become corrupted: if the other end was disconnected, if the fd was closed and so on. If we ignore this condition, then we enter a infinite loop. Instead, when the fd becomes invalid, unregister the corresponding worker and notify the callback.
Regarding pure, when this is merged, I will do a pull request to pure to take this failure mode into account. |
Oh, that's awesome. Looking forward to it, and thanks again for this 😄. |
and restart automatically the worker when it dies (see mafredri/zsh-async#31)
In some cases, the fd created by zpty can become corrupted: if the other
end was disconnected, if the fd was closed and so on.
If we ignore this condition, then we enter a infinite loop. This eats 100% of a cpu and is very annoying.
Instead, when the fd becomes invalid, this commit unregisters the corresponding
worker and notifies the callback.
The error code was chosen to be "[async]" because the readme mentions this is how some existing errors are already notified to callbacks.
I have discovered this problem as part of the pure prompt. This happens randomly about once a week. I have been using this fork for some time, now, and the problem has disappeared.
The tests still pass.