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

prevent infinite loop in case of broken zpty file descriptor #31

Merged
merged 1 commit into from
Dec 14, 2018

Conversation

symphorien
Copy link
Contributor

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.

@symphorien
Copy link
Contributor Author

The ci build failure seems spurious to me. Please tell me if there is something to fix.

Copy link
Owner

@mafredri mafredri left a 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
Copy link
Owner

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.

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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.
@symphorien
Copy link
Contributor Author

Regarding pure, when this is merged, I will do a pull request to pure to take this failure mode into account.

@mafredri
Copy link
Owner

Oh, that's awesome. Looking forward to it, and thanks again for this 😄.

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

2 participants