From 3cf3563b77d70ae5ac14763330fe98b71397f557 Mon Sep 17 00:00:00 2001 From: Ayman Bagabas Date: Fri, 28 Jul 2023 12:17:22 -0400 Subject: [PATCH] fix!: output race condition 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: https://github.com/charmbracelet/lipgloss/pull/210 Fixes: https://github.com/muesli/termenv/issues/145 --- copy.go | 4 +-- output.go | 37 ++++++++++++++----- output_test.go | 22 ++++++++++++ screen.go | 88 +++++++++++++++++++++++----------------------- templatehelper.go | 4 +-- termenv.go | 11 ++++-- termenv_other.go | 8 ++--- termenv_test.go | 14 ++++---- termenv_unix.go | 10 +++--- termenv_windows.go | 6 ++-- 10 files changed, 127 insertions(+), 77 deletions(-) create mode 100644 output_test.go diff --git a/copy.go b/copy.go index 4bf5c9f..e2ac2bf 100644 --- a/copy.go +++ b/copy.go @@ -7,7 +7,7 @@ import ( ) // Copy copies text to clipboard using OSC 52 escape sequence. -func (o Output) Copy(str string) { +func (o *Output) Copy(str string) { s := osc52.New(str) if strings.HasPrefix(o.environ.Getenv("TERM"), "screen") { s = s.Screen() @@ -17,7 +17,7 @@ func (o Output) Copy(str string) { // CopyPrimary copies text to primary clipboard (X11) using OSC 52 escape // sequence. -func (o Output) CopyPrimary(str string) { +func (o *Output) CopyPrimary(str string) { s := osc52.New(str).Primary() if strings.HasPrefix(o.environ.Getenv("TERM"), "screen") { s = s.Screen() diff --git a/output.go b/output.go index e22d369..d7245b6 100644 --- a/output.go +++ b/output.go @@ -22,7 +22,7 @@ type OutputOption = func(*Output) // Output is a terminal output. type Output struct { - Profile + profile Profile tty io.Writer environ Environ @@ -33,6 +33,8 @@ type Output struct { fgColor Color bgSync *sync.Once bgColor Color + + mtx sync.RWMutex } // Environ is an interface for getting environment variables. @@ -66,7 +68,7 @@ func NewOutput(tty io.Writer, opts ...OutputOption) *Output { o := &Output{ tty: tty, environ: &osEnviron{}, - Profile: -1, + profile: -1, fgSync: &sync.Once{}, fgColor: NoColor{}, bgSync: &sync.Once{}, @@ -79,8 +81,8 @@ func NewOutput(tty io.Writer, opts ...OutputOption) *Output { for _, opt := range opts { opt(o) } - if o.Profile < 0 { - o.Profile = o.EnvColorProfile() + if o.profile < 0 { + o.profile = o.EnvColorProfile() } return o @@ -96,7 +98,7 @@ func WithEnvironment(environ Environ) OutputOption { // WithProfile returns a new OutputOption for the given profile. func WithProfile(profile Profile) OutputOption { return func(o *Output) { - o.Profile = profile + o.profile = profile } } @@ -133,6 +135,21 @@ func WithUnsafe() OutputOption { } } +// ColorProfile returns the color profile. +// Ascii, ANSI, ANSI256, or TrueColor. +func (o *Output) ColorProfile() Profile { + o.mtx.RLock() + defer o.mtx.RUnlock() + return o.profile +} + +// SetColorProfile sets the color profile. +func (o *Output) SetColorProfile(profile Profile) { + o.mtx.Lock() + defer o.mtx.Unlock() + o.profile = profile +} + // ForegroundColor returns the terminal's default foreground color. func (o *Output) ForegroundColor() Color { f := func() { @@ -140,7 +157,9 @@ func (o *Output) ForegroundColor() Color { return } + o.mtx.Lock() o.fgColor = o.foregroundColor() + o.mtx.Unlock() } if o.cache { @@ -159,7 +178,9 @@ func (o *Output) BackgroundColor() Color { return } + o.mtx.Lock() o.bgColor = o.backgroundColor() + o.mtx.Unlock() } if o.cache { @@ -180,18 +201,18 @@ func (o *Output) HasDarkBackground() bool { // TTY returns the terminal's file descriptor. This may be nil if the output is // not a terminal. -func (o Output) TTY() File { +func (o *Output) TTY() File { if f, ok := o.tty.(File); ok { return f } return nil } -func (o Output) Write(p []byte) (int, error) { +func (o *Output) Write(p []byte) (int, error) { return o.tty.Write(p) } // WriteString writes the given string to the output. -func (o Output) WriteString(s string) (int, error) { +func (o *Output) WriteString(s string) (int, error) { return o.Write([]byte(s)) } diff --git a/output_test.go b/output_test.go new file mode 100644 index 0000000..f300530 --- /dev/null +++ b/output_test.go @@ -0,0 +1,22 @@ +package termenv + +import ( + "io" + "testing" +) + +func TestOutputRace(t *testing.T) { + o := NewOutput(io.Discard) + for i := 0; i < 100; i++ { + t.Run("Test race", func(t *testing.T) { + t.Parallel() + o.Write([]byte("test")) + o.SetColorProfile(ANSI) + o.ColorProfile() + o.HasDarkBackground() + o.TTY() + o.ForegroundColor() + o.BackgroundColor() + }) + } +} diff --git a/screen.go b/screen.go index a71181b..ee80824 100644 --- a/screen.go +++ b/screen.go @@ -70,234 +70,234 @@ const ( ) // Reset the terminal to its default style, removing any active styles. -func (o Output) Reset() { +func (o *Output) Reset() { fmt.Fprint(o.tty, CSI+ResetSeq+"m") } // SetForegroundColor sets the default foreground color. -func (o Output) SetForegroundColor(color Color) { +func (o *Output) SetForegroundColor(color Color) { fmt.Fprintf(o.tty, OSC+SetForegroundColorSeq, color) } // SetBackgroundColor sets the default background color. -func (o Output) SetBackgroundColor(color Color) { +func (o *Output) SetBackgroundColor(color Color) { fmt.Fprintf(o.tty, OSC+SetBackgroundColorSeq, color) } // SetCursorColor sets the cursor color. -func (o Output) SetCursorColor(color Color) { +func (o *Output) SetCursorColor(color Color) { fmt.Fprintf(o.tty, OSC+SetCursorColorSeq, color) } // RestoreScreen restores a previously saved screen state. -func (o Output) RestoreScreen() { +func (o *Output) RestoreScreen() { fmt.Fprint(o.tty, CSI+RestoreScreenSeq) } // SaveScreen saves the screen state. -func (o Output) SaveScreen() { +func (o *Output) SaveScreen() { fmt.Fprint(o.tty, CSI+SaveScreenSeq) } // AltScreen switches to the alternate screen buffer. The former view can be // restored with ExitAltScreen(). -func (o Output) AltScreen() { +func (o *Output) AltScreen() { fmt.Fprint(o.tty, CSI+AltScreenSeq) } // ExitAltScreen exits the alternate screen buffer and returns to the former // terminal view. -func (o Output) ExitAltScreen() { +func (o *Output) ExitAltScreen() { fmt.Fprint(o.tty, CSI+ExitAltScreenSeq) } // ClearScreen clears the visible portion of the terminal. -func (o Output) ClearScreen() { +func (o *Output) ClearScreen() { fmt.Fprintf(o.tty, CSI+EraseDisplaySeq, 2) o.MoveCursor(1, 1) } // MoveCursor moves the cursor to a given position. -func (o Output) MoveCursor(row int, column int) { +func (o *Output) MoveCursor(row int, column int) { fmt.Fprintf(o.tty, CSI+CursorPositionSeq, row, column) } // HideCursor hides the cursor. -func (o Output) HideCursor() { +func (o *Output) HideCursor() { fmt.Fprint(o.tty, CSI+HideCursorSeq) } // ShowCursor shows the cursor. -func (o Output) ShowCursor() { +func (o *Output) ShowCursor() { fmt.Fprint(o.tty, CSI+ShowCursorSeq) } // SaveCursorPosition saves the cursor position. -func (o Output) SaveCursorPosition() { +func (o *Output) SaveCursorPosition() { fmt.Fprint(o.tty, CSI+SaveCursorPositionSeq) } // RestoreCursorPosition restores a saved cursor position. -func (o Output) RestoreCursorPosition() { +func (o *Output) RestoreCursorPosition() { fmt.Fprint(o.tty, CSI+RestoreCursorPositionSeq) } // CursorUp moves the cursor up a given number of lines. -func (o Output) CursorUp(n int) { +func (o *Output) CursorUp(n int) { fmt.Fprintf(o.tty, CSI+CursorUpSeq, n) } // CursorDown moves the cursor down a given number of lines. -func (o Output) CursorDown(n int) { +func (o *Output) CursorDown(n int) { fmt.Fprintf(o.tty, CSI+CursorDownSeq, n) } // CursorForward moves the cursor up a given number of lines. -func (o Output) CursorForward(n int) { +func (o *Output) CursorForward(n int) { fmt.Fprintf(o.tty, CSI+CursorForwardSeq, n) } // CursorBack moves the cursor backwards a given number of cells. -func (o Output) CursorBack(n int) { +func (o *Output) CursorBack(n int) { fmt.Fprintf(o.tty, CSI+CursorBackSeq, n) } // CursorNextLine moves the cursor down a given number of lines and places it at // the beginning of the line. -func (o Output) CursorNextLine(n int) { +func (o *Output) CursorNextLine(n int) { fmt.Fprintf(o.tty, CSI+CursorNextLineSeq, n) } // CursorPrevLine moves the cursor up a given number of lines and places it at // the beginning of the line. -func (o Output) CursorPrevLine(n int) { +func (o *Output) CursorPrevLine(n int) { fmt.Fprintf(o.tty, CSI+CursorPreviousLineSeq, n) } // ClearLine clears the current line. -func (o Output) ClearLine() { +func (o *Output) ClearLine() { fmt.Fprint(o.tty, CSI+EraseEntireLineSeq) } // ClearLineLeft clears the line to the left of the cursor. -func (o Output) ClearLineLeft() { +func (o *Output) ClearLineLeft() { fmt.Fprint(o.tty, CSI+EraseLineLeftSeq) } // ClearLineRight clears the line to the right of the cursor. -func (o Output) ClearLineRight() { +func (o *Output) ClearLineRight() { fmt.Fprint(o.tty, CSI+EraseLineRightSeq) } // ClearLines clears a given number of lines. -func (o Output) ClearLines(n int) { +func (o *Output) ClearLines(n int) { clearLine := fmt.Sprintf(CSI+EraseLineSeq, 2) cursorUp := fmt.Sprintf(CSI+CursorUpSeq, 1) fmt.Fprint(o.tty, clearLine+strings.Repeat(cursorUp+clearLine, n)) } // ChangeScrollingRegion sets the scrolling region of the terminal. -func (o Output) ChangeScrollingRegion(top, bottom int) { +func (o *Output) ChangeScrollingRegion(top, bottom int) { fmt.Fprintf(o.tty, CSI+ChangeScrollingRegionSeq, top, bottom) } // InsertLines inserts the given number of lines at the top of the scrollable // region, pushing lines below down. -func (o Output) InsertLines(n int) { +func (o *Output) InsertLines(n int) { fmt.Fprintf(o.tty, CSI+InsertLineSeq, n) } // DeleteLines deletes the given number of lines, pulling any lines in // the scrollable region below up. -func (o Output) DeleteLines(n int) { +func (o *Output) DeleteLines(n int) { fmt.Fprintf(o.tty, CSI+DeleteLineSeq, n) } // EnableMousePress enables X10 mouse mode. Button press events are sent only. -func (o Output) EnableMousePress() { +func (o *Output) EnableMousePress() { fmt.Fprint(o.tty, CSI+EnableMousePressSeq) } // DisableMousePress disables X10 mouse mode. -func (o Output) DisableMousePress() { +func (o *Output) DisableMousePress() { fmt.Fprint(o.tty, CSI+DisableMousePressSeq) } // EnableMouse enables Mouse Tracking mode. -func (o Output) EnableMouse() { +func (o *Output) EnableMouse() { fmt.Fprint(o.tty, CSI+EnableMouseSeq) } // DisableMouse disables Mouse Tracking mode. -func (o Output) DisableMouse() { +func (o *Output) DisableMouse() { fmt.Fprint(o.tty, CSI+DisableMouseSeq) } // EnableMouseHilite enables Hilite Mouse Tracking mode. -func (o Output) EnableMouseHilite() { +func (o *Output) EnableMouseHilite() { fmt.Fprint(o.tty, CSI+EnableMouseHiliteSeq) } // DisableMouseHilite disables Hilite Mouse Tracking mode. -func (o Output) DisableMouseHilite() { +func (o *Output) DisableMouseHilite() { fmt.Fprint(o.tty, CSI+DisableMouseHiliteSeq) } // EnableMouseCellMotion enables Cell Motion Mouse Tracking mode. -func (o Output) EnableMouseCellMotion() { +func (o *Output) EnableMouseCellMotion() { fmt.Fprint(o.tty, CSI+EnableMouseCellMotionSeq) } // DisableMouseCellMotion disables Cell Motion Mouse Tracking mode. -func (o Output) DisableMouseCellMotion() { +func (o *Output) DisableMouseCellMotion() { fmt.Fprint(o.tty, CSI+DisableMouseCellMotionSeq) } // EnableMouseAllMotion enables All Motion Mouse mode. -func (o Output) EnableMouseAllMotion() { +func (o *Output) EnableMouseAllMotion() { fmt.Fprint(o.tty, CSI+EnableMouseAllMotionSeq) } // DisableMouseAllMotion disables All Motion Mouse mode. -func (o Output) DisableMouseAllMotion() { +func (o *Output) DisableMouseAllMotion() { fmt.Fprint(o.tty, CSI+DisableMouseAllMotionSeq) } // EnableMouseExtendedMotion enables Extended Mouse mode (SGR). This should be // enabled in conjunction with EnableMouseCellMotion, and EnableMouseAllMotion. -func (o Output) EnableMouseExtendedMode() { +func (o *Output) EnableMouseExtendedMode() { fmt.Fprint(o.tty, CSI+EnableMouseExtendedModeSeq) } // DisableMouseExtendedMotion disables Extended Mouse mode (SGR). -func (o Output) DisableMouseExtendedMode() { +func (o *Output) DisableMouseExtendedMode() { fmt.Fprint(o.tty, CSI+DisableMouseExtendedModeSeq) } // EnableMousePixelsMotion enables Pixel Motion Mouse mode (SGR-Pixels). This // should be enabled in conjunction with EnableMouseCellMotion, and // EnableMouseAllMotion. -func (o Output) EnableMousePixelsMode() { +func (o *Output) EnableMousePixelsMode() { fmt.Fprint(o.tty, CSI+EnableMousePixelsModeSeq) } // DisableMousePixelsMotion disables Pixel Motion Mouse mode (SGR-Pixels). -func (o Output) DisableMousePixelsMode() { +func (o *Output) DisableMousePixelsMode() { fmt.Fprint(o.tty, CSI+DisableMousePixelsModeSeq) } // SetWindowTitle sets the terminal window title. -func (o Output) SetWindowTitle(title string) { +func (o *Output) SetWindowTitle(title string) { fmt.Fprintf(o.tty, OSC+SetWindowTitleSeq, title) } // EnableBracketedPaste enables bracketed paste. -func (o Output) EnableBracketedPaste() { +func (o *Output) EnableBracketedPaste() { fmt.Fprintf(o.tty, CSI+EnableBracketedPasteSeq) } // DisableBracketedPaste disables bracketed paste. -func (o Output) DisableBracketedPaste() { +func (o *Output) DisableBracketedPaste() { fmt.Fprintf(o.tty, CSI+DisableBracketedPasteSeq) } diff --git a/templatehelper.go b/templatehelper.go index d75b079..57a33b2 100644 --- a/templatehelper.go +++ b/templatehelper.go @@ -5,8 +5,8 @@ import ( ) // TemplateFuncs returns template helpers for the given output. -func (o Output) TemplateFuncs() template.FuncMap { - return TemplateFuncs(o.Profile) +func (o *Output) TemplateFuncs() template.FuncMap { + return TemplateFuncs(o.ColorProfile()) } // TemplateFuncs contains a few useful template helpers. diff --git a/termenv.go b/termenv.go index 4ceb271..f29f4b8 100644 --- a/termenv.go +++ b/termenv.go @@ -44,6 +44,11 @@ func ColorProfile() Profile { return output.ColorProfile() } +// SetColorProfile sets the color profile. +func SetColorProfile(profile Profile) { + output.SetColorProfile(profile) +} + // ForegroundColor returns the terminal's default foreground color. func ForegroundColor() Color { return output.ForegroundColor() @@ -80,7 +85,7 @@ func EnvNoColor() bool { // EnvColorProfile returns the color profile based on environment variables set // Supports NO_COLOR (https://no-color.org/) // and CLICOLOR/CLICOLOR_FORCE (https://bixense.com/clicolors/) -// If none of these environment variables are set, this behaves the same as ColorProfile() +// If none of these environment variables are set, it tries to detect the color profile from TERM. // It will return the Ascii color profile if EnvNoColor() returns true // If the terminal does not support any colors, but CLICOLOR_FORCE is set and not "0" // then the ANSI color profile will be returned. @@ -91,7 +96,7 @@ func EnvColorProfile() Profile { // EnvColorProfile returns the color profile based on environment variables set // Supports NO_COLOR (https://no-color.org/) // and CLICOLOR/CLICOLOR_FORCE (https://bixense.com/clicolors/) -// If none of these environment variables are set, this behaves the same as ColorProfile() +// If none of these environment variables are set, it tries to detect the color profile from TERM. // It will return the Ascii color profile if EnvNoColor() returns true // If the terminal does not support any colors, but CLICOLOR_FORCE is set and not "0" // then the ANSI color profile will be returned. @@ -99,7 +104,7 @@ func (o *Output) EnvColorProfile() Profile { if o.EnvNoColor() { return Ascii } - p := o.ColorProfile() + p := o.termColorProfile() if o.cliColorForced() && p == Ascii { return ANSI } diff --git a/termenv_other.go b/termenv_other.go index 93a43b6..2200a45 100644 --- a/termenv_other.go +++ b/termenv_other.go @@ -5,18 +5,18 @@ package termenv import "io" -// ColorProfile returns the supported color profile: +// colorProfile returns the supported color profile: // ANSI256 -func (o Output) ColorProfile() Profile { +func (o *Output) termColorProfile() Profile { return ANSI256 } -func (o Output) foregroundColor() Color { +func (o *Output) foregroundColor() Color { // default gray return ANSIColor(7) } -func (o Output) backgroundColor() Color { +func (o *Output) backgroundColor() Color { // default black return ANSIColor(0) } diff --git a/termenv_test.go b/termenv_test.go index 3f750d6..8e8030d 100644 --- a/termenv_test.go +++ b/termenv_test.go @@ -36,8 +36,9 @@ func TestLegacyTermEnv(t *testing.T) { func TestTermEnv(t *testing.T) { o := NewOutput(os.Stdout) - if o.Profile != TrueColor && o.Profile != Ascii { - t.Errorf("Expected %d, got %d", TrueColor, o.Profile) + p := o.ColorProfile() + if p != TrueColor && p != Ascii { + t.Errorf("Expected %d, got %d", TrueColor, p) } fg := o.ForegroundColor() @@ -360,8 +361,9 @@ func TestEnvNoColor(t *testing.T) { func TestPseudoTerm(t *testing.T) { buf := &bytes.Buffer{} o := NewOutput(buf) - if o.Profile != Ascii { - t.Errorf("Expected %d, got %d", Ascii, o.Profile) + p := o.ColorProfile() + if p != Ascii { + t.Errorf("Expected %d, got %d", Ascii, p) } fg := o.ForegroundColor() @@ -377,8 +379,8 @@ func TestPseudoTerm(t *testing.T) { } exp := "foobar" - out := o.String(exp) - out = out.Foreground(o.Color("#abcdef")) + out := p.String(exp) + out = out.Foreground(p.Color("#abcdef")) o.Write([]byte(out.String())) if buf.String() != exp { diff --git a/termenv_unix.go b/termenv_unix.go index 24d519a..f32b33d 100644 --- a/termenv_unix.go +++ b/termenv_unix.go @@ -18,9 +18,9 @@ const ( OSCTimeout = 5 * time.Second ) -// ColorProfile returns the supported color profile: +// termColorProfile returns the supported color profile: // Ascii, ANSI, ANSI256, or TrueColor. -func (o *Output) ColorProfile() Profile { +func (o *Output) termColorProfile() Profile { if !o.isTTY() { return Ascii } @@ -69,7 +69,7 @@ func (o *Output) ColorProfile() Profile { return Ascii } -func (o Output) foregroundColor() Color { +func (o *Output) foregroundColor() Color { s, err := o.termStatusReport(10) if err == nil { c, err := xTermColor(s) @@ -91,7 +91,7 @@ func (o Output) foregroundColor() Color { return ANSIColor(7) } -func (o Output) backgroundColor() Color { +func (o *Output) backgroundColor() Color { s, err := o.termStatusReport(11) if err == nil { c, err := xTermColor(s) @@ -223,7 +223,7 @@ func (o *Output) readNextResponse() (response string, isOSC bool, err error) { return "", false, ErrStatusReport } -func (o Output) termStatusReport(sequence int) (string, error) { +func (o *Output) termStatusReport(sequence int) (string, error) { // screen/tmux can't support OSC, because they can be connected to multiple // terminals concurrently. term := o.environ.Getenv("TERM") diff --git a/termenv_windows.go b/termenv_windows.go index 1d9c618..a1b933c 100644 --- a/termenv_windows.go +++ b/termenv_windows.go @@ -10,7 +10,7 @@ import ( "golang.org/x/sys/windows" ) -func (o *Output) ColorProfile() Profile { +func (o *Output) termColorProfile() Profile { if !o.isTTY() { return Ascii } @@ -43,12 +43,12 @@ func (o *Output) ColorProfile() Profile { return TrueColor } -func (o Output) foregroundColor() Color { +func (o *Output) foregroundColor() Color { // default gray return ANSIColor(7) } -func (o Output) backgroundColor() Color { +func (o *Output) backgroundColor() Color { // default black return ANSIColor(0) }