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

Long pause/halt when lipgloss is determining background color #73

Open
jefferai opened this issue Mar 20, 2022 · 7 comments
Open

Long pause/halt when lipgloss is determining background color #73

jefferai opened this issue Mar 20, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@jefferai
Copy link

Sometimes when running my program (very similar to the fancy list demo and using the same lipgloss variables), when my list is created, I see a very long pause. It doesn't happen all the time -- over half the time, but not every time -- but that pause can take anywhere from a few seconds to 30 seconds or more. I got a SIGQUIT stack trace -- it's down in termenv so let me know if you'd rather I post there than here, but since my entrypoint there is lipgloss I figured I'd start here.

goroutine 1 [syscall]:
syscall.syscall(0x10060cbe0, 0x1, 0x14000390077, 0x1)
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/runtime/sys_darwin.go:22 +0x1c
syscall.read(0x1, {0x14000390077, 0x1, 0x1})
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/syscall/zsyscall_darwin_arm64.go:1171 +0x5c
syscall.Read(...)
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/syscall/syscall_unix.go:189
internal/poll.ignoringEINTRIO(...)
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/internal/poll/fd_unix.go:582
internal/poll.(*FD).Read(0x140001a0060, {0x14000390077, 0x1, 0x1})
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/internal/poll/fd_unix.go:163 +0x214
os.(*File).read(...)
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/os/file_posix.go:32
os.(*File).Read(0x1400019e008, {0x14000390077, 0x1, 0x1})
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/os/file.go:119 +0x74
github.com/muesli/termenv.readNextByte(0x1400019e008)
        /Users/jeff/go/pkg/mod/github.com/muesli/termenv@v0.11.1-0.20220212125758-44cd13922739/termenv_unix.go:135 +0x98
github.com/muesli/termenv.readNextResponse(0x1400019e008)
        /Users/jeff/go/pkg/mod/github.com/muesli/termenv@v0.11.1-0.20220212125758-44cd13922739/termenv_unix.go:185 +0x1c8
github.com/muesli/termenv.termStatusReport(0xb)
        /Users/jeff/go/pkg/mod/github.com/muesli/termenv@v0.11.1-0.20220212125758-44cd13922739/termenv_unix.go:246 +0x22c
github.com/muesli/termenv.backgroundColor()
        /Users/jeff/go/pkg/mod/github.com/muesli/termenv@v0.11.1-0.20220212125758-44cd13922739/termenv_unix.go:85 +0x28
github.com/muesli/termenv.BackgroundColor()
        /Users/jeff/go/pkg/mod/github.com/muesli/termenv@v0.11.1-0.20220212125758-44cd13922739/termenv.go:67 +0x6c
github.com/muesli/termenv.HasDarkBackground()
        /Users/jeff/go/pkg/mod/github.com/muesli/termenv@v0.11.1-0.20220212125758-44cd13922739/termenv.go:72 +0x20
github.com/charmbracelet/lipgloss.HasDarkBackground.func1()
        /Users/jeff/go/pkg/mod/github.com/charmbracelet/lipgloss@v0.5.0/color.go:60 +0x20
sync.(*Once).doSlow(0x1070a68a8, 0x10653e450)
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/sync/once.go:68 +0x10c
sync.(*Once).Do(...)
        /opt/homebrew/Cellar/go/1.17.8/libexec/src/sync/once.go:59
github.com/charmbracelet/lipgloss.HasDarkBackground()
        /Users/jeff/go/pkg/mod/github.com/charmbracelet/lipgloss@v0.5.0/color.go:59 +0x5c
github.com/charmbracelet/lipgloss.AdaptiveColor.value(...)
        /Users/jeff/go/pkg/mod/github.com/charmbracelet/lipgloss@v0.5.0/color.go:167
github.com/charmbracelet/lipgloss.AdaptiveColor.color({{0x101437372, 0x7}, {0x101437491, 0x7}})
        /Users/jeff/go/pkg/mod/github.com/charmbracelet/lipgloss@v0.5.0/color.go:174 +0x2c
github.com/charmbracelet/lipgloss.Style.Render({0x140006749f0, {0x101432dc9, 0x3}}, {0x101432dc9, 0x3})
        /Users/jeff/go/pkg/mod/github.com/charmbracelet/lipgloss@v0.5.0/style.go:230 +0x10e0
github.com/charmbracelet/lipgloss.Style.String(...)
        /Users/jeff/go/pkg/mod/github.com/charmbracelet/lipgloss@v0.5.0/style.go:109
github.com/charmbracelet/bubbles/list.New({0x0, 0x0, 0x0}, {0x106585f38, 0x140000dea90}, 0x0, 0x0)
        /Users/jeff/go/pkg/mod/github.com/charmbracelet/bubbles@v0.10.3/list/list.go:165 +0x2a4
<my code>

Let me know what other info I can provide. Thanks!

@muesli
Copy link
Member

muesli commented Mar 20, 2022

Hey @jefferai, long time no see!

termenv is querying the terminal for its current settings via control sequences on stdout, and the terminal is expected to respond by printing to stdout, which termenv reads from. There's a timeout of 5 seconds defined, after which the read should be aborted: https://github.com/muesli/termenv/blob/master/termenv_unix.go#L112

It looks like waitForData is reporting data to be available, but then the read call blocks waiting for a single byte. I haven't seen that on any of my (test-) systems so far.

Could you verify whether bytes are actually slowly trickling in or if it's stuck there waiting for a response that'll never arrive?

@jefferai
Copy link
Author

jefferai commented Mar 20, 2022

Hi @muesli 😀

It's definitely not 5 seconds. Sometimes it's 2 or 3; often it's 10 or so; sometimes (very rarely) it's 30 or more. I don't think I've seen it completely freeze forever.

How would you want me to verify that bytes are trickling in? Just fork the lib and print debug out?

I can also see if it happens in other envs. I'm running on arm64 M1 on Monterey 12.2.1 in iTerm using zsh. I did try with bash without any difference. Stock terminal makes no difference.

@bashbunni bashbunni added the question Further information is requested label Apr 8, 2022
@ahmedxfn
Copy link

Same exact issue here, this issue appeared when I tried the component Help from https://github.com/charmbracelet/bubbles, after some investigating, I figured out that when removing lipgloss.JoinHorizontal from this line and this line, plus removing the renderer from here, it works without freezing.

@ahmedxfn
Copy link

After more investigating, the real cause for me was from lipgloss.AdaptiveColor which call termenv.HasDarkBackground() to determine the background color as @jefferai stated, replacing the adaptive color with normal color solved the issue.

@muesli muesli added the bug Something isn't working label Apr 19, 2022
@muesli
Copy link
Member

muesli commented Jun 2, 2022

@r4ndomx That observation is correct, yeah. I think I finally figured out under which circumstances this is happening: if the first background detection happens only after bubbletea has taken over control of stdin/out, the communication channel between terminal and termenv is essentially broken. You could work around this issue by manually calling lipgloss.SetHasDarkBackground(termenv.HasDarkBackground()) once before initializing your bubbletea app.

We'll clean this up and may have to refactor the API a bit here. See upcoming work in next branch.

@muesli muesli self-assigned this Jun 2, 2022
@muesli muesli removed the question Further information is requested label Oct 6, 2022
DavidS-ovm added a commit to overmindtech/cli that referenced this issue Jun 3, 2024
It turns out that there is a lipgloss/termenv/bubbletea/reality
incompatibility when querying the background color. By querying this once
at the start before bubbletea takes control of the terminal, this can be
avoided.

See charmbracelet/lipgloss#73 (comment) for details.
@mikecbone
Copy link

Is there any update on having this issue fixed within lipgloss itself?

My understanding is that the issue arises because Bubbletea holds a lock on the os.Stdout file descriptor, which prevents lipgloss from performing terminal background color checks required for adaptive colors.

Calling lipgloss.SetHasDarkBackground(termenv.HasDarkBackground()) does indeed fix the issue. A potential alternative solution might involve using a shared mutex to synchronize access to the file descriptor, but this would only be needed if the background color was changing (though I'm sure there's also some caching involved that would need to be updated too).

In the meantime, I think it would be helpful to update the README to mention this issue under the Adaptive Colors section. This would save other developers a lot of time in figuring out the problem.

Thank you!

@meowgorithm
Copy link
Member

@mikecbone Noted! For what it's worth we have some upcoming features that both run background color detection in a non-blocking way and let you opt out of it entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants