Segments randomly missing from the cache

[lewis-yeung]

For cross-segment template references, OMP caches segment data via (*environment.TemplateCache).AddSegmentData. However, this does not seem to work well. I tested it (e.g., {{.Segments.Exit.Meaning}}) on Windows/Linux and I found that every time OMP updates the cache file, some segments are randomly missing from the cached Segments map, which makes a cross-segment template not always available.

[lewis-yeung]

I've looked through the code but couldn't find the source of the problem.

[JanDeDobbeleer]

I honestly never noticed that, have multiple segments with this functionality. What could happen is that when using multiple sessions (tabs, etc), it overwrites the cache and it doesn't really work for transient or another. Other than that, I don't see an immediate reason why this would occur.

[lewis-yeung]

@JanDeDobbeleer I tested it in PowerShell by continuously pressing Ctrl + L to redraw the prompt, and I noticed that an expected 0 didn't always show up in the time segment where I utilized {{.Segments.Exit.Meaning}}.

The custom theme I use for tests:

palette:
  darkOliveGreen: "#556b2f"
  chocolate: "#d2691e"
  teal: "#008080"
  dodgerBlue: "#1e90ff"
blocks:
  - alignment: left
    segments:
      - foreground: white
        style: plain
        template: ╭─
        type: text
      - background: blue
        foreground: white
        leading_diamond: 
        style: diamond
        template: " {{if .WSL}}[<lightGreen>WSL</>]{{end}}{{.Icon}}  <lightYellow>{{.Shell}}</> "
        type: os
      - background: white
        foreground: black
        powerline_symbol: 
        style: powerline
        template: " {{if .SSHSession}} {{end}}{{if .Root}} {{else}} {{end}}<p:dodgerBlue><b>{{.UserName}}</b></><green>@</> <p:chocolate><b>{{.HostName}}</b></> "
        type: session
      - background: p:teal
        foreground: white
        style: powerline
        powerline_symbol: 
        template: "{{.Segments.Exit.Meaning}}  {{.CurrentDate | date .Format}} "
        type: time
      - background_templates:
          - "{{if ne .Code 0}}red{{else}}green{{end}}"
        foreground: white
        properties:
          always_enabled: true
        powerline_symbol: 
        style: powerline
        template: " {{if ne .Code 0}} {{.Code}}{{else}} ✔{{end}} "
        type: exit
      - background: p:darkOliveGreen
        foreground: white
        properties:
          always_enabled: true
        powerline_symbol: 
        style: powerline
        template: "{{if ne .Ms 0}}  {{.FormattedMs}} {{end}}"
        type: executiontime
      - background: magenta
        foreground: white
        properties:
          style: folder
          mapped_locations_enabled: false
          mapped_locations:
            "~": "~"
        style: diamond
        template: " {{if gt .StackCount 0}}[<lightGreen><b>{{.StackCount}}</b></>] {{end}} {{.Parent}}<lightGreen>{{.Path}}</> "
        trailing_diamond: 
        type: path
    type: prompt
  - alignment: left
    newline: true
    segments:
      - style: plain
        template: "╰─<lightGreen></> "
        type: text
    type: prompt
console_title_template: "{{if .Root}}*{{end}}{{.Shell}} | {{.UserName}}@{{.HostName}} | {{.PWD}}"
secondary_prompt:
  foreground: darkGray
  template: "»»» "
valid_line:
  foreground: lightGreen
  template: " "
error_line:
  foreground: lightRed
  template: " "
transient_prompt:
  template: "<lightGreen> </> "
version: 2

[lewis-yeung]

@JanDeDobbeleer Have you tested this case? I just kept pressing Ctrl + L in PowerShell to spawn new prompts and it was still broken with the latest version on my end.

[lewis-yeung]

Besides, I found the bug occurs more often on Windows (broken cases > normal cases) than on Linux (broken cases < normal cases), and I'm sure it's not shell-specific.

[JanDeDobbeleer]

@lewis-yeung I didn't validate this yet, no. Could be that concurrent writes to the cache cause this.

[lewis-yeung]

Could be that concurrent writes to the cache cause this.

After some debugging and tests, I finally found the culprit for this. 👻 It's not exactly a concurrent cache-writing issue, but associated with concurrency (Goroutines) indeed. Let's consider this call hierarchy:

(*engine.Block).setEnabledSegments -(1)-> (*engine.Segment).SetEnabled -(2)-> (*platform.Shell).TemplateCache

The keypoint is that env.TemplateCache() (actually, Shell.tmplCache) in (2) does not always point to the same instance of TemplateCache.

In (1), one Goroutine is created for each segment's SetEnabled logic. Of course, this check will fail for the first call. After a new instance of TemplateCache is created in a Goroutine but before env.tmplCache = tmplCache is executed, another Goroutine does not know its existence, so the above check may still fail and the early return will not be executed. That way different Goroutines may create more than one instance of TemplateCache in total, but only one will be finally preserved and be written to the cache file (omp.cache), and not all segments are added into this instance of TemplateCache. Thus once a new prompt is generated, there can be some segments randomly missing from the cached Segments map.

Make a change in block.go like the following (i.e., to remove the concurrency) is a feasible solution: (validated)

--- a/src/engine/block.go
+++ b/src/engine/block.go
@@ -109,10 +109,8 @@ func (b *Block) setEnabledSegments() {
        wg.Add(len(b.Segments))
        defer wg.Wait()
        for _, segment := range b.Segments {
-               go func(s *Segment) {
-                       defer wg.Done()
-                       s.SetEnabled(b.env)
-               }(segment)
+               segment.SetEnabled(b.env)
+               wg.Done()
        }
 }

Perhaps there is a better option. @JanDeDobbeleer Any ideas?

[JanDeDobbeleer]

@lewis-yeung we just need to sync the creation and assignment of the templatecache in this case.

[lewis-yeung]

@JanDeDobbeleer Will you fix it?

[lewis-yeung]

@JanDeDobbeleer I noticed that you had tried to fix this using R/W lock inside (*platform.Shell).TemplateCache. Actually, I had already tested this way and knew it could introduce a deadlock before I posted the long comment above. Sorry that I didn't mention this earlier. So as an alternative, should we just remove the concurrency in (*engine.Block).setEnabledSegments? Or should we add a specific R/W lock in the type definition of TemplateCache instead of just using a global one? IMO, it's not a good idea to share a global lock between methods of different data types (Shell, TemplateCache and concurrentMap), though in a way it can be by design.

[JanDeDobbeleer]

@lewis-yeung we can indeed try a local one. I'm not sure why I designed it this way, can be the wrong solution. I want to keep the parallelism as it's s bit faster.

[JanDeDobbeleer]

@lewis-yeung I refactored that and fixed it. It was because PWD() also locks and that one's called from TemplateCache() so that lock could be removed as we call TemplateCache() before PWD()

[lewis-yeung]

@JanDeDobbeleer I noticed that this bug has reappeared since the commit 4e19379 in which you tried to fix another bug.

[JanDeDobbeleer]

Maybe we should then not execute in parallel. Not sure if there's a lot of benefit anymore nowadays.

[lewis-yeung]

I have got a fix that also preserves concurrency, will put it up later. :)

[lewis-yeung]

@JanDeDobbeleer The problem reappears in v24. Still broken in v24.0.11. :(

[JanDeDobbeleer]

@lewis-yeung I tried to reproduce this last time and couldn't, theoretically, when the segment is enabled, it will also be added to the template cache.

[lewis-yeung]

Using a minimal example config as below:

version: 3
patch_pwsh_bleed: true
final_space: true
blocks:
  - type: prompt
    alignment: left
    segments:
      - type: text
        alias: foo
        style: plain
        template: '***'
      - type: text
        style: plain
        template: ->{{.Segments.foo.Text}}<-
      - type: text
        style: plain
        template: bar|
      - type: text
        style: plain
        template: baz|

I tested on Windows with just holding Enter to emit new prompts, and got some prompts with *** missing between -> and <-:

@JanDeDobbeleer As my previous comment said, it can be at least easily reproduced on Windows, but hardly on Linux without adding more segments.

I guess there may be a similar root cause.

[JanDeDobbeleer]

@lewis-yeung ok cool, I can indeed reproduce it. It's a race condition. In this case, as .Text is available only after rendering, the check to render is before that happens. Now, theoretically, a select statement is blocking. So I'll have a look at why this happens.

[JanDeDobbeleer]

@lewis-yeung found the issue, can't reproduce it anymore now.

[lewis-yeung]

@JanDeDobbeleer Confirmed to be resolved. Thanks.