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

fix: renderer race condition #210

Merged
merged 1 commit into from Aug 1, 2023
Merged

fix: renderer race condition #210

merged 1 commit into from Aug 1, 2023

Conversation

aymanbagabas
Copy link
Member

@aymanbagabas aymanbagabas commented Jul 27, 2023

Guard accessing the underlying Termenv output behind a mutex. Multiple goroutines can set/get the dark background color causing a race condition.

Needs: muesli/termenv#146

renderer_test.go Show resolved Hide resolved
renderer.go Show resolved Hide resolved
aymanbagabas added a commit to charmbracelet/soft-serve that referenced this pull request Jul 27, 2023
aymanbagabas added a commit to charmbracelet/soft-serve that referenced this pull request Jul 27, 2023
aymanbagabas added a commit to charmbracelet/soft-serve that referenced this pull request Jul 28, 2023
aymanbagabas added a commit to charmbracelet/soft-serve that referenced this pull request Jul 28, 2023
aymanbagabas added a commit to muesli/termenv that referenced this pull request Jul 28, 2023
Make output profile accessible through `ColorProfile`.
Use `SetColorProfile` to change the output color profile.
Use a mutex to guard output writes.

Fixes: charmbracelet/lipgloss#210
Fixes: #145
aymanbagabas added a commit to muesli/termenv that referenced this pull request Jul 28, 2023
Make output profile accessible through `ColorProfile`.
Use `SetColorProfile` to change the output color profile.
Use a mutex to guard output writes.
Use pointer receiver

Fixes: charmbracelet/lipgloss#210
Fixes: #145
aymanbagabas added a commit to muesli/termenv that referenced this pull request Jul 28, 2023
BREAKING!

`ColorProfile` reads the terminal environment every time the function is
called. This is inefficient. We only need to read the `$TERM`
environment variable once when we initialize the output. So we cache
the value we read.

Rename `ColorProfile` to `termColorProfile` and rely on
`EnvColorProfile` to detect the profile. Ideally, we rely on the
terminal Terminfo to detect the profile and fallback to the environment.

Make output profile accessible through `ColorProfile`.
Use `SetColorProfile` to change the output color profile.
Use a mutex to guard output writes.
Use pointer receiver since we have a lock in the struct

Fixes: charmbracelet/lipgloss#210
Fixes: #145
aymanbagabas added a commit to muesli/termenv that referenced this pull request Jul 28, 2023
`ColorProfile` reads the terminal environment every time the function is
called. This is inefficient. We only need to read the `$TERM`
environment variable once when we initialize the output. So instead, we cache
the value we read.

Rename `ColorProfile` to `termColorProfile` and rely on
`EnvColorProfile` to detect the profile. Ideally, we would rely on the
terminal Terminfo to detect the profile and fallback to the environment.
But that's for another day :)

Make output profile accessible through `ColorProfile`.
Use `SetColorProfile` to change the output color profile.
Use a mutex to guard output writes.
Use pointer receiver since we have a lock in the struct

Fixes: charmbracelet/lipgloss#210
Fixes: #145
aymanbagabas added a commit to muesli/termenv that referenced this pull request Jul 28, 2023
`ColorProfile` reads the terminal environment every time the function is
called. This is inefficient. We only need to read the `$TERM`
environment variable once when we initialize the output. So instead, we cache
the value we read.

Rename `ColorProfile` to `termColorProfile` and rely on
`EnvColorProfile` to detect the profile. Ideally, we would rely on the
terminal Terminfo to detect the profile and fallback to the environment.
But that's for another day :)

Make output profile accessible through `ColorProfile`.
Use `SetColorProfile` to change the output color profile.
Use a mutex to guard output writes.
Use pointer receiver since we have a lock in the struct

Fixes: charmbracelet/lipgloss#210
Fixes: #145
@aymanbagabas aymanbagabas force-pushed the fix-race branch 2 times, most recently from ae7c722 to 4100381 Compare July 28, 2023 18:59
Guard accessing the underlying Termenv output behind a mutex. Multiple goroutines can set/get the dark background color causing a race condition.

Needs: muesli/termenv#146
aymanbagabas added a commit to muesli/termenv that referenced this pull request Jul 28, 2023
Use a mutex to guard setting/getting the color profile and fg/bg colors.
Use `SetColorProfile` to change the output color profile.
Use pointer receiver since we have a lock in the struct.

Fixes: charmbracelet/lipgloss#210
Fixes: #145
aymanbagabas added a commit to muesli/termenv that referenced this pull request Jul 28, 2023
Use a mutex to guard setting/getting the color profile and fg/bg colors.
Use `SetColorProfile` to change the output color profile.
Use pointer receiver since we have a lock in the struct.

Fixes: charmbracelet/lipgloss#210
Fixes: #145
Copy link
Member

@muesli muesli left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@aymanbagabas aymanbagabas merged commit ac8231e into master Aug 1, 2023
27 checks passed
@aymanbagabas aymanbagabas deleted the fix-race branch August 1, 2023 12:23
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

3 participants