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 most data race issues & enable in CI tests #447

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/go.yml
Expand Up @@ -31,8 +31,11 @@ jobs:
- name: Build
run: go build -v .

- name: Test Race
run: go test -race -p 1 .
MarvinJWendt marked this conversation as resolved.
Show resolved Hide resolved

- name: Test
run: go test -coverprofile="coverage.txt" -covermode=atomic -p 1 .
run: go test -race -coverprofile="coverage.txt" -covermode=atomic -p 1 .

- name: Upload coverage to Codecov
if: success() && matrix.os == 'ubuntu-latest'
Expand Down
2 changes: 1 addition & 1 deletion barchart.go
Expand Up @@ -245,7 +245,7 @@ func (p BarChartPrinter) Srender() (string, error) {
}
// =================================================================================================================

if RawOutput {
if RawOutput.Load() {
return p.getRawOutput(), nil
}
for i, bar := range p.Bars {
Expand Down
2 changes: 1 addition & 1 deletion bigtext_printer.go
Expand Up @@ -74,7 +74,7 @@ func (p BigTextPrinter) WithWriter(writer io.Writer) *BigTextPrinter {
func (p BigTextPrinter) Srender() (string, error) {
var ret string

if RawOutput {
if RawOutput.Load() {
for _, letter := range p.Letters {
ret += letter.String
}
Expand Down
2 changes: 1 addition & 1 deletion center_printer.go
Expand Up @@ -37,7 +37,7 @@ func (p CenterPrinter) WithWriter(writer io.Writer) *CenterPrinter {
// Sprint formats using the default formats for its operands and returns the resulting string.
// Spaces are added between operands when neither is a string.
func (p CenterPrinter) Sprint(a ...interface{}) string {
if RawOutput {
if RawOutput.Load() {
return Sprint(a...)
}

Expand Down
16 changes: 12 additions & 4 deletions color.go
Expand Up @@ -12,12 +12,16 @@ var PrintColor = true

// EnableColor enables colors.
func EnableColor() {
pLock.Lock()
defer pLock.Unlock()
color.Enable = true
PrintColor = true
}

// DisableColor disables colors.
func DisableColor() {
pLock.Lock()
defer pLock.Unlock()
color.Enable = false
PrintColor = false
}
Expand Down Expand Up @@ -148,10 +152,12 @@ func (c Color) Sprintln(a ...interface{}) string {
// Spaces are added between operands when neither is a string.
// Input will be colored with the parent Color.
func (c Color) Sprint(a ...interface{}) string {
message := Sprint(a...)
pLock.Lock()
defer pLock.Unlock()
message := color.Sprint(a...)
messageLines := strings.Split(message, "\n")
for i, line := range messageLines {
messageLines[i] = color.RenderCode(c.String(), strings.ReplaceAll(line, color.ResetSet, Sprintf("\x1b[0m\u001B[%sm", c.String())))
messageLines[i] = color.RenderCode(c.String(), strings.ReplaceAll(line, color.ResetSet, color.Sprintf("\x1b[0m\u001B[%sm", c.String())))
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for changing this to use color directly?

Copy link
Author

Choose a reason for hiding this comment

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

This all revolves around how the color lib is NOT thread safe at all. A lot of the Data Races revolved around it and I needed to add the print lock to this function as it leverages the color lib directly so I needed to change that out since Sprintf also has a lock on it so we would get recursive locking and deadlock

Copy link
Author

Choose a reason for hiding this comment

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

IMO I may have been a little lazy here and I was trying not to be too intrusive. The use of the color lib should be all wrapped up in one sub package so we can use it everywhere safely without this type of thing because its not super clear why this is necessary, could be a good follow up.

Copy link
Member

Choose a reason for hiding this comment

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

so I needed to change that out since Sprintf also has a lock on it so we would get recursive locking and deadlock

Ok, that makes sense!

The use of the color lib should be all wrapped up in one sub package

Honestly, I often thought about completely switching the color library, or writing an own implementation. There are so many workarounds that we did in the past 2 years, because of it. Sadly, back in the days when PTerm was young and only created to be used in one of my projects, I used the color library to create the Style, RGB, FgXxx and BgXxx colors. So it's not easy to switch it.

Copy link
Author

Choose a reason for hiding this comment

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

yeah makes sense. Probably part of a bigger effort, just another reason to write a custom implementation.

}
message = strings.Join(messageLines, "\n")
return message
Expand Down Expand Up @@ -280,10 +286,12 @@ func (s Style) Add(styles ...Style) Style {
// Spaces are added between operands when neither is a string.
// Input will be colored with the parent Style.
func (s Style) Sprint(a ...interface{}) string {
message := Sprint(a...)
pLock.Lock()
defer pLock.Unlock()
message := color.Sprint(a...)
messageLines := strings.Split(message, "\n")
for i, line := range messageLines {
messageLines[i] = color.RenderCode(s.String(), strings.ReplaceAll(line, color.ResetSet, Sprintf("\x1b[0m\u001B[%sm", s.String())))
messageLines[i] = color.RenderCode(s.String(), strings.ReplaceAll(line, color.ResetSet, color.Sprintf("\x1b[0m\u001B[%sm", s.String())))
}
message = strings.Join(messageLines, "\n")
return color.RenderCode(s.String(), message)
Expand Down
2,009 changes: 1,141 additions & 868 deletions coverage.txt

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions go.mod
Expand Up @@ -21,5 +21,6 @@ require (
github.com/rivo/uniseg v0.2.0 // indirect
github.com/sergi/go-diff v1.2.0 // indirect
github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778 // indirect
go.uber.org/atomic v1.10.0 // indirect
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect
)
2 changes: 2 additions & 0 deletions go.sum
Expand Up @@ -59,6 +59,8 @@ github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PK
github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778 h1:QldyIu/L63oPpyvQmHgvgickp1Yw510KJOqX7H24mg8=
github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778/go.mod h1:2MuV+tbUrU1zIOPMxZ5EncGwgmMJsa+9ucAQZXxsObs=
go.uber.org/atomic v1.10.0 h1:9qC72Qh0+3MqyJbAn8YU5xVq1frD8bn3JtD2oXtafVQ=
go.uber.org/atomic v1.10.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Expand Down
2 changes: 1 addition & 1 deletion header_printer.go
Expand Up @@ -64,7 +64,7 @@ func (p HeaderPrinter) WithWriter(writer io.Writer) *HeaderPrinter {
// Sprint formats using the default formats for its operands and returns the resulting string.
// Spaces are added between operands when neither is a string.
func (p HeaderPrinter) Sprint(a ...interface{}) string {
if RawOutput {
if RawOutput.Load() {
return Sprint(a...)
}

Expand Down
2 changes: 2 additions & 0 deletions interactive_confirm_printer_test.go
@@ -1,3 +1,5 @@
//go:build !race
MarvinJWendt marked this conversation as resolved.
Show resolved Hide resolved

package pterm_test

import (
Expand Down
2 changes: 2 additions & 0 deletions interactive_continue_printer_test.go
@@ -1,3 +1,5 @@
//go:build !race

package pterm_test

import (
Expand Down
2 changes: 2 additions & 0 deletions interactive_multiselect_printer_test.go
@@ -1,3 +1,5 @@
//go:build !race

package pterm_test

import (
Expand Down
2 changes: 2 additions & 0 deletions interactive_select_printer_test.go
@@ -1,3 +1,5 @@
//go:build !race

package pterm_test

import (
Expand Down
2 changes: 2 additions & 0 deletions interactive_textinput_printer_test.go
@@ -1,3 +1,5 @@
//go:build !race

package pterm_test

import (
Expand Down
2 changes: 1 addition & 1 deletion panel_printer.go
Expand Up @@ -89,7 +89,7 @@ func (p PanelPrinter) getRawOutput() string {
func (p PanelPrinter) Srender() (string, error) {
var ret string

if RawOutput {
if RawOutput.Load() {
return p.getRawOutput(), nil
}

Expand Down
2 changes: 1 addition & 1 deletion paragraph_printer.go
Expand Up @@ -34,7 +34,7 @@ func (p ParagraphPrinter) WithWriter(writer io.Writer) *ParagraphPrinter {
// Sprint formats using the default formats for its operands and returns the resulting string.
// Spaces are added between operands when neither is a string.
func (p ParagraphPrinter) Sprint(a ...interface{}) string {
if RawOutput {
if RawOutput.Load() {
return Sprint(a...)
}

Expand Down
18 changes: 9 additions & 9 deletions prefix_printer.go
Expand Up @@ -156,11 +156,11 @@ func (p PrefixPrinter) WithWriter(writer io.Writer) *PrefixPrinter {
// Spaces are added between operands when neither is a string.
func (p *PrefixPrinter) Sprint(a ...interface{}) string {
m := Sprint(a...)
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return ""
}

if RawOutput {
if RawOutput.Load() {
if p.Prefix.Text != "" {
return Sprintf("%s: %s", strings.TrimSpace(p.Prefix.Text), Sprint(a...))
} else {
Expand Down Expand Up @@ -215,7 +215,7 @@ func (p *PrefixPrinter) Sprint(a ...interface{}) string {
// Sprintln formats using the default formats for its operands and returns the resulting string.
// Spaces are always added between operands and a newline is appended.
func (p PrefixPrinter) Sprintln(a ...interface{}) string {
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return ""
}
str := fmt.Sprintln(a...)
Expand All @@ -224,7 +224,7 @@ func (p PrefixPrinter) Sprintln(a ...interface{}) string {

// Sprintf formats according to a format specifier and returns the resulting string.
func (p PrefixPrinter) Sprintf(format string, a ...interface{}) string {
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return ""
}
return p.Sprint(Sprintf(format, a...))
Expand All @@ -233,7 +233,7 @@ func (p PrefixPrinter) Sprintf(format string, a ...interface{}) string {
// Sprintfln formats according to a format specifier and returns the resulting string.
// Spaces are always added between operands and a newline is appended.
func (p PrefixPrinter) Sprintfln(format string, a ...interface{}) string {
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return ""
}
return p.Sprintf(format, a...) + "\n"
Expand All @@ -244,7 +244,7 @@ func (p PrefixPrinter) Sprintfln(format string, a ...interface{}) string {
// It returns the number of bytes written and any write error encountered.
func (p *PrefixPrinter) Print(a ...interface{}) *TextPrinter {
tp := TextPrinter(p)
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return &tp
}
p.LineNumberOffset--
Expand All @@ -259,7 +259,7 @@ func (p *PrefixPrinter) Print(a ...interface{}) *TextPrinter {
// It returns the number of bytes written and any write error encountered.
func (p *PrefixPrinter) Println(a ...interface{}) *TextPrinter {
tp := TextPrinter(p)
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return &tp
}
Fprint(p.Writer, p.Sprintln(a...))
Expand All @@ -271,7 +271,7 @@ func (p *PrefixPrinter) Println(a ...interface{}) *TextPrinter {
// It returns the number of bytes written and any write error encountered.
func (p *PrefixPrinter) Printf(format string, a ...interface{}) *TextPrinter {
tp := TextPrinter(p)
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return &tp
}
Fprint(p.Writer, p.Sprintf(format, a...))
Expand All @@ -284,7 +284,7 @@ func (p *PrefixPrinter) Printf(format string, a ...interface{}) *TextPrinter {
// It returns the number of bytes written and any write error encountered.
func (p *PrefixPrinter) Printfln(format string, a ...interface{}) *TextPrinter {
tp := TextPrinter(p)
if p.Debugger && !PrintDebugMessages {
if p.Debugger && !PrintDebugMessages.Load() {
return &tp
}
p.LineNumberOffset++
Expand Down
60 changes: 38 additions & 22 deletions print.go
Expand Up @@ -4,29 +4,42 @@ import (
"fmt"
"io"
"strings"
"sync"

"github.com/gookit/color"
)

// Need to use this because "github.com/gookit/color" is NOT a thread-safe library for Print & Sprintf functions.
// Used to protect against some unsafe actions in Fprint as well
var pLock sync.RWMutex
Copy link
Member

Choose a reason for hiding this comment

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

Could you please rename this to something like printLock?


// SetDefaultOutput sets the default output of pterm.
func SetDefaultOutput(w io.Writer) {
pLock.Lock()
defer pLock.Unlock()
color.SetOutput(w)
}

// Sprint formats using the default formats for its operands and returns the resulting string.
// Spaces are added between operands when neither is a string.
func Sprint(a ...interface{}) string {
pLock.Lock()
defer pLock.Unlock()
return color.Sprint(a...)
}

// Sprintf formats according to a format specifier and returns the resulting string.
func Sprintf(format string, a ...interface{}) string {
pLock.Lock()
defer pLock.Unlock()
return color.Sprintf(format, a...)
}

// Sprintfln formats according to a format specifier and returns the resulting string.
// Spaces are always added between operands and a newline is appended.
func Sprintfln(format string, a ...interface{}) string {
pLock.Lock()
defer pLock.Unlock()
return color.Sprintf(format, a...) + "\n"
}

Expand Down Expand Up @@ -98,44 +111,43 @@ func PrintOnErrorf(format string, a ...interface{}) {
// Spaces are added between operands when neither is a string.
// It returns the number of bytes written and any write error encountered.
func Fprint(writer io.Writer, a ...interface{}) {
if !Output {
pLock.Lock()
defer pLock.Unlock()
if !Output.Load() {
return
}

var ret string
var printed bool

for _, bar := range ActiveProgressBarPrinters {
activeProgressBarPrinters.lock.Lock()
for _, bar := range activeProgressBarPrinters.printers {
if bar.IsActive && bar.Writer == writer {
ret += sClearLine()
ret += Sprinto(a...)
ret += "\r" + color.Sprint(a...)
Comment on lines -111 to +127
Copy link
Member

Choose a reason for hiding this comment

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

Is there are particular reason for not using Sprinto?

Copy link
Author

Choose a reason for hiding this comment

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

yeah same as previous comments, would cause a deadlock

Copy link
Member

Choose a reason for hiding this comment

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

Could you please add that as a comment? I think that might be confusing for others at first glance. :)

printed = true
}
}
activeProgressBarPrinters.lock.Unlock()

for _, spinner := range activeSpinnerPrinters {
if spinner.IsActive && spinner.Writer == writer {
activeSpinnerPrinters.lock.Lock()
for _, spinner := range activeSpinnerPrinters.printers {
if spinner.atomicIsActive.Load() && spinner.Writer == writer {
ret += sClearLine()
ret += Sprinto(a...)
ret += "\r" + color.Sprint(a...)
printed = true
}
}
activeSpinnerPrinters.lock.Unlock()

if !printed {
ret = color.Sprint(Sprint(a...))
ret = color.Sprint(a...)
}

if writer != nil {
color.Fprint(writer, Sprint(ret))
color.Fprint(writer, color.Sprint(ret))
} else {
color.Print(Sprint(ret))
}

// Refresh all progressbars in case they were overwritten previously. Reference: #302
MarvinJWendt marked this conversation as resolved.
Show resolved Hide resolved
for _, bar := range ActiveProgressBarPrinters {
if bar.IsActive {
bar.UpdateTitle(bar.Title)
}
color.Print(color.Sprint(ret))
}
}

Expand All @@ -154,22 +166,26 @@ func Fprintln(writer io.Writer, a ...interface{}) {
// time.Sleep(time.Second)
// pterm.Printo("Hello, Earth!")
func Printo(a ...interface{}) {
if !Output {
pLock.Lock()
defer pLock.Unlock()
if !Output.Load() {
return
}

color.Print("\r" + Sprint(a...))
color.Print("\r" + color.Sprint(a...))
}

// Fprinto prints Printo to a custom writer.
func Fprinto(w io.Writer, a ...interface{}) {
if !Output {
pLock.Lock()
defer pLock.Unlock()
if !Output.Load() {
return
}
if w != nil {
color.Fprint(w, "\r", Sprint(a...))
color.Fprint(w, "\r", color.Sprint(a...))
} else {
color.Print("\r", Sprint(a...))
color.Print("\r", color.Sprint(a...))
}
}

Expand All @@ -183,5 +199,5 @@ func fClearLine(writer io.Writer) {
}

func sClearLine() string {
return Sprinto(strings.Repeat(" ", GetTerminalWidth()))
return "\r" + color.Sprint(strings.Repeat(" ", GetTerminalWidth()))
}