Fix background jobs segment

[dritter]

The background_jobs segment now reports the jobs count immediately. We trap CLD (Child process was killed) here, taken from here. Caveat: It might cause some trouble, if in multiline-prompts (cursor position changes, when we show the segment).

Performancewise, this redraws the complete prompt on every child exit (solvable in #1176 ).

I haven't tested it much (running out of time again), and I am not a fan of trapping signals all over, but that seems to be our only chance..

// cc @Syphdias Fixes #1196 . Succeeds #1238 .

[dritter]

Olay. Thanks for review. Let's roll with that for a while and see how it turns out.

[romkatv]

There are corner cases that exhibit interesting behavior. E.g., this:

unsetopt monitor
function TRAPALRM() { true & }
TMOUT=1

And of course there is the obvious problem of claiming a singleton resource (SIGCLD handler) so that no one else can use it.

[dritter]

@romkatv Thanks for the input. Yes, I think there are edge cases. Not sure how common these use cases are..

About singleton SIGCLD trap: I experimented with localtraps, but as far as I understood them, the trap is as long local as the outer function runs. So that wouldn't help us here. Another option I consider is attaching our code to an already existing one (if it exists). But that isn't ideal either: We can only attach to existing traps. If a trap for that signal was defined later, it would overwrite our definition. Do you know any other way for implementing a trap in a safe way?

[romkatv]

Do you know any other way for implementing a trap in a safe way?

I'm afraid it's not possible in zsh. Global traps are off limits for plugins.

At some point the workaround I used in Powerlevel10k was to embed a small script in PROMPT that has a side effect when PROMPT is expanded for the second time. The logic was as follows:

1. Set PROMPT in precmd hook.
2. Do nothing when PROMPT is expanded for the first time. This is the normal case.
3. When a background job completes, it re-expands PROMPT, which gets detected by the embedded script, which in turn assigned a new value to PROMPT and calls zle reset-prompt.

Here's a proof of concept:

❯ cat /tmp/poc/.zshrc 
function set_prompt() {
  typeset -ig N=0
  local render_hook='${${${$((++N)):#2}:-$(echo second rendering >&2)}+}'
  local actual_prompt='%m%# '
  PROMPT="${render_hook}${actual_prompt}"
}

setopt promptsubst
autoload -Uz add-zsh-hook
add-zsh-hook precmd set_prompt

❯ ZDOTDIR=/tmp/poc zsh

If you start a background job, you'll see "second rendering" printed to the console when it completes.

Note that PROMPT contains $(...). That parts gets executed only on the second rendering of the prompt. The first rendering is free of process substitutions, so it won't exhibit unsavory behavior on Ctrl+C. Whatever you put in $(...) should be very fast to make it unlikely that it gets interrupted by Ctrl+C on the second rendering (this isn't a big deal though).

Unfortunately, you cannot simply replace echo second rendering with set_prompt && zle reset-prompt because zle isn't active at that point (it's a subshell after all). With a bit of perseverance you can overcome this difficulty.

P.S.

It seems like these are the options for fixing background jobs in next.

1. Leave background_jobs broken. This is a regression compared to master. On the plus side, the behavior is predictable and the code is simple.
2. Revert the change that broke background_jobs. That is, put $(build_left_prompt) back in PROMPT. Choosing between (1) and (2) is a judgement call w.r.t. user preferences. Do users prefer correct background_jobs over correct behavior on Ctrl+C or the other way around? If correct Ctrl+C is more important, are its advantages big enough to change status quo?
3. Trap SIGCLD. This will work in most cases. On the other hand, it can cause hard-to-debug issues for some users. Global traps in plugins are time bombs.
4. Detect when PROMPT is expanded for the second time. This is quite complex and it'll still look worse on next than on master because prompt will get rendered twice when background jobs complete.
5. Do what Powerlevel10k does. This gives the desired behavior from the users' point of view but the code is very complex.

[Syphdias]

local render_hook='${${${$((++N)):#2}:-$(echo second rendering >&2)}+}'

This is ingenious (as ever)! I at least was missing the means to detect that second render.

Unfortunately, you cannot simply replace echo second rendering with set_prompt && zle reset-prompt because zle isn't active at that point (it's a subshell after all). With a bit of perseverance you can overcome this difficulty.

You could do it with a handler:

diff --git a/generator/default.p9k b/generator/default.p9k
index 718f6d7f..959ab9ca 100644
--- a/generator/default.p9k
+++ b/generator/default.p9k
@@ -437,7 +437,9 @@ local NEWLINE='
   # For security $PROMPT is never set directly. This way the prompt render is
   # forced to evaluate the variable and the contents of $__p9k_unsafe_PROMPT
   # are never executed. The same applies to $RPROMPT.
-  PROMPT='$__p9k_unsafe_PROMPT'
+  typeset -ig N=0
+  local render_hook='${${${$((++N)):#2}:-$( echo here we could trigger a handler >&2)}+}'
+  PROMPT="$render_hook"'${__p9k_unsafe_PROMPT}'
 }

 p9k::set_default P9K_IGNORE_TERM_COLORS false

Sidenote: When I attempted to fix the ctrl-c behavior I ran into the trouble of double expansion which lead to unwanted behavior like $(./evil-git-branch-name.sh) being executed. I circumvented this by putting it into another variable. In retrospective, this might not have been the best idea since it very limiting; quoting every prompt return would have been the better choice, I think. I'm not sure if this would make this unnecessary but probably easier.

[romkatv]

You could do it with a handler

What do you mean by handler?

[Syphdias]

What do you mean by handler?

I was thinking something similar to the continually updating clock (zle -F $fd $handler) but without the need for a daemon/clock and just trigger it with the second render detection.
Apparently, this didn't work in a quick test. The prompt got redrawn on cursor movement or any other input but not by itself.

[romkatv]

FWIW, the only way I managed to implement this handler is with the help of another process. Not pretty.

[Syphdias]

Yeah, but in theory, the extra process just writes to the fifo every second to trigger the handler which isn't needed here. Not sure why it worked differently in my test, maybe I messed up somewhere.

[romkatv]

I don't think you messed up anywhere. If you write to the file during prompt expansion, the handler simply won't be called (bugs in zsh). You can revive the handler by sending zsh a signal (I used SIGWINCH because it has default signal handler that doesn't do anything) but you need to do it when zle is active, meaning after a short delay.

[Syphdias]

@romkatv Thanks for the hint. Little PoC, with no clean up done:

diff --git a/generator/default.p9k b/generator/default.p9k
index 718f6d7f..b8947c7b 100644
--- a/generator/default.p9k
+++ b/generator/default.p9k
@@ -437,7 +437,9 @@ local NEWLINE='
   # For security $PROMPT is never set directly. This way the prompt render is
   # forced to evaluate the variable and the contents of $__p9k_unsafe_PROMPT
   # are never executed. The same applies to $RPROMPT.
-  PROMPT='$__p9k_unsafe_PROMPT'
+  typeset -ig N=0
+  local render_hook='${${${$((++N)):#2}:-$( zsh -c "sleep 1 && kill -WINCH $$ && echo" >'"$fifo"' 2>&1 &! )}+}'
+  PROMPT="$render_hook"'${__p9k_unsafe_PROMPT}'
 }

 p9k::set_default P9K_IGNORE_TERM_COLORS false
diff --git a/powerlevel9k.zsh-theme b/powerlevel9k.zsh-theme
index 4cbc2318..095c9883 100755
--- a/powerlevel9k.zsh-theme
+++ b/powerlevel9k.zsh-theme
@@ -189,6 +189,22 @@ esac
 p9k::defined P9K_LEFT_PROMPT_ELEMENTS || P9K_LEFT_PROMPT_ELEMENTS=(context dir vcs)
 p9k::defined P9K_RIGHT_PROMPT_ELEMENTS || P9K_RIGHT_PROMPT_ELEMENTS=(status root_indicator background_jobs history time)

+###########
+# PoC
+###########
+declare -g fifo
+fifo=$(mktemp -u "${TMPDIR:-/tmp}"/p9k.$$.pipe.time.XXXXXXXXXX)
+mkfifo $fifo
+exec {__p9k_prompt_reset_hook}<>$fifo
+typeset -gfH __p9k_reset_prompt() {
+  # reset prompt on pipe update
+  emulate -L zsh
+  local _
+  IFS='' read -u $__p9k_prompt_reset_hook _ \
+    && __p9k_background_jobs && __p9k_prepare_prompts && zle && zle .reset-prompt
+}
+zle -F $__p9k_prompt_reset_hook __p9k_reset_prompt
+
 ################################################################
 # Load Prompt Segment Definitions
 ################################################################

[romkatv]

Little PoC, with no clean up done:

:+1:

[Syphdias]

@dritter I think @romkatv listed pretty much all the possibilities in https://github.com/bhilburn/powerlevel9k/pull/1254#issuecomment-485711853. What do you think? What should we go with? Fixed enough for now and live with the consequences until #1176 or something else?

[dritter]

Sorry for the delay. And thanks @romkatv for explaining the options in detail.

Leave background_jobs broken. This is a regression compared to master. On the plus side, the behavior is predictable and the code is simple.

Well, the fix with the trap is already merged. We could revert that, obviously, but I would rather go with the trap.

Revert the change that broke background_jobs. That is, put $(build_left_prompt) back in PROMPT. Choosing between (1) and (2) is a judgement call w.r.t. user preferences. Do users prefer correct background_jobs over correct behavior on Ctrl+C or the other way around? If correct Ctrl+C is more important, are its advantages big enough to change status quo?

That would be a no-go for me. I've seen more issues about incorrect CTRL-C behavior than about background_jobs not working right.

Trap SIGCLD. This will work in most cases. On the other hand, it can cause hard-to-debug issues for some users. Global traps in plugins are time bombs.

That is the status quo now in next. I agree that this is not ideal..

Detect when PROMPT is expanded for the second time. This is quite complex and it'll still look worse on next than on master because prompt will get rendered twice when background jobs complete.

This is probably a good solution, if we can render the segments individually. But that is - as @Syphdias suggests - something for #1176 .

Do what Powerlevel10k does. This gives the desired behavior from the users' point of view but the code is very complex.

That is somewhat too extreme for my taste.

So, overall I would live with the trap for the moment, and add render detection once we have #1176 . WDYT @Syphdias ?

[Syphdias]

So, overall I would live with the trap for the moment, and add render detection once we have #1176 .

I agree. I would rather react fast to complaints than to delay further. I'm still a fan of speeding up the release cycle (a lot).