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.

Sign up for GitHub

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

Jump to bottom

fix(async): avoid blocking the shell while waiting #12304

Merged
merged 3 commits into from Apr 3, 2024
Merged

fix(async): avoid blocking the shell while waiting #12304

merged 3 commits into from Apr 3, 2024

Conversation

hwti
Copy link
Contributor

@hwti hwti commented Mar 25, 2024

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.
  • If the code introduces new aliases, I provide a valid use case for all plugin users down below.

Changes:

Other comments:

This is draft for now, as I cannot explain why the subshell could sometimes block for several seconds (which blocked the shell in _omz_async_callback).
Even when it didn't block, the subshell was 10-20ms slower anyway.

@hwti
fix(async): avoid blocking the shell while waiting
42b22ec 
Due to "builtin echo $handler", _omz_async_callback was called immediately, as there was data to
read on the pipe. Then the shell was blocked until the end of the async prompt handler.
We now use associative array to get the handler name from the fd.

For an unknown reason, the async child can block when launching a subshell, especially if there are
several handlers registered.
Using a function to set the return code avoids the issue, and is much faster.

Fixes ohmyzsh#12290
@ohmyzsh ohmyzsh bot added the Area: core Issue or PR related to core parts of the project label Mar 25, 2024
@hwti hwti changed the title fix(async): avoid blocking the shell while waiting (#12290) fix(async): avoid blocking the shell while waiting Mar 25, 2024
@mcornella mcornella mentioned this pull request Apr 3, 2024
Closed
@mcornella mcornella marked this pull request as ready for review April 3, 2024 17:35
mcornella
mcornella approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@mcornella mcornella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks fast and snappy, and works well with 5.0.8. LGTM!

@mcornella mcornella merged commit b43b84a into ohmyzsh:master Apr 3, 2024
2 checks passed
@mcornella
Copy link
Member

Thanks Loïc!

@hwti
Copy link
Contributor Author

hwti commented Apr 3, 2024

@mcornella You squashed, so the fork improvements are in the same commit as the bugfix, and there is no comment any more (there is still the PR, but...).

I didn't know there was a way to search in the keys of _OMZ_ASYNC_FDS[(r).

IIRC, the read pid <&$fd => read -u $fd is just a small optimization (avoiding a few dup / close syscalls).
So the only fork you removed is the $(cat ).

@hwti hwti deleted the fix-async-prompt-freeze branch April 3, 2024 21:12
pull bot pushed a commit to kis87988/ohmyzsh that referenced this pull request Apr 4, 2024
@hwti @mcornella @pull
fix(async): avoid blocking the shell while waiting (ohmyzsh#12304)
a3175d0 
Co-authored-by: Marc Cornellà <marc@mcornella.com>
BougaCaillou added a commit to BougaCaillou/ohmybouga that referenced this pull request Apr 8, 2024
@BougaCaillou
Merge remote-tracking branch 'upstream/master'
3b90955 
* upstream/master:
  Update README for git-prompt to clarify how to use (ohmyzsh#10922)
  fix(autojump): add macOS 'pkgsrc' installation location (ohmyzsh#12329)
  fix(lib/git): fix detection of function use in prompt
  perf(async): avoid executing `true` if not required (ohmyzsh#12318)
  fix(git): disable locally `ksharrays`
  fix(archlinux): make `upgrade` work with non-english
  feat(git): implement async completion for `git_prompt_status` (ohmyzsh#12319)
  feat(git)!: enable async git prompt (now for real)
  fix(async): avoid blocking the shell while waiting (ohmyzsh#12304)
  feat(httpie): complete https command (ohmyzsh#12314)
  feat(gcloud): add homebrew installation path (ohmyzsh#12308)
  fix(starship): keep `ZSH_THEME` if not installed (ohmyzsh#12309)
  style: remove trailing whitespace (ohmyzsh#12303)
  fix(poetry-env): activate only if env exists (ohmyzsh#12301)
  feat(terraform): add `tft` (`terraform test`) (ohmyzsh#12299)
  feat(jira): add help/usage cmd (ohmyzsh#12293)
  fix(ssh-agent): don't start new agent if screen/tmux symlink exists (ohmyzsh#12297)
lukaselmer pushed a commit to lukaselmer/ohmyzsh that referenced this pull request Apr 16, 2024
@hwti @mcornella @lukaselmer
fix(async): avoid blocking the shell while waiting (ohmyzsh#12304)
0382bbb 
Co-authored-by: Marc Cornellà <marc@mcornella.com>
cschuyle pushed a commit to cschuyle/ohmyzsh that referenced this pull request Apr 18, 2024
@hwti @mcornella @cschuyle
fix(async): avoid blocking the shell while waiting (ohmyzsh#12304)
132b92c 
Co-authored-by: Marc Cornellà <marc@mcornella.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcornella mcornella mcornella approved these changes

Assignees

@mcornella mcornella

Labels
Area: core Issue or PR related to core parts of the project
Projects
Main project
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Freezes when using more than one async prompt handler
2 participants
@hwti @mcornella