Fix most data race issues & enable in CI tests #447
Conversation
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
===========================================
- Coverage 86.07% 72.68% -13.39%
===========================================
Files 29 29
Lines 2119 2175 +56
===========================================
- Hits 1824 1581 -243
- Misses 264 591 +327
+ Partials 31 3 -28
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Looks like there are a few data races that still need to be addressed, will update this. |
fde0869
to
12ebc30
Compare
| // Output completely disables output from pterm if set to false. Can be used in CLI application quiet mode. | ||
| Output = true | ||
| Output = atomic.NewBool(true) | ||
|
|
||
| // PrintDebugMessages sets if messages printed by the DebugPrinter should be printed. | ||
| PrintDebugMessages = false | ||
| PrintDebugMessages = atomic.NewBool(false) | ||
|
|
||
| // RawOutput is set to true if pterm.DisableStyling() was called. | ||
| // The variable indicates that PTerm will not add additional styling to text. | ||
| // Use pterm.DisableStyling() or pterm.EnableStyling() to change this variable. | ||
| // Changing this variable directly, will disable or enable the output of colored text. | ||
| RawOutput = false | ||
| RawOutput = atomic.NewBool(false) |
There was a problem hiding this comment.
Went back and forth on this part but this is the only place where the API actually changed. If I implemented with internal atomic variables like I did elsewhere it could lead to other issues. IMO these should not be exposed publicly anyways as it can lead to inconsistent usage. Looks like it may have only been done for the tests
There was a problem hiding this comment.
I think we should just expose a getter and a setter function for the atomic variables. Ubers atomic package provides much more functionality than needed. Also, if we ever decide to switch it, we would have another breaking change. A simple wrapper would fix that :)
// EnableStyling enables the default PTerm styling.
// This also calls EnableColor.
func EnableStyling() {
RawOutput.Store(false)
EnableColor()
}
// DisableStyling sets PTerm to RawOutput mode and disables all of PTerms styling.
// You can use this to print to text files etc.
// This also calls DisableColor.
func DisableStyling() {
RawOutput.Store(true)
DisableColor()
}Those functions can be used, but I would also introduce SetStylingEnabled(bool) and GetStylingEnabled(). We could get rid of the exposed RawOutput then.
The same for the other variables.
There was a problem hiding this comment.
Agreed, will change this around. No need for a braking change here I think either.
| 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()))) |
There was a problem hiding this comment.
What is the reason for changing this to use color directly?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah makes sense. Probably part of a bigger effort, just another reason to write a custom implementation.
|
|
||
| "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 |
There was a problem hiding this comment.
Could you please rename this to something like printLock?
| ret += Sprinto(a...) | ||
| ret += "\r" + color.Sprint(a...) |
There was a problem hiding this comment.
Is there are particular reason for not using Sprinto?
There was a problem hiding this comment.
yeah same as previous comments, would cause a deadlock
There was a problem hiding this comment.
Could you please add that as a comment? I think that might be confusing for others at first glance. :)
|
Also just want to explain the race detector is finicky. It does not always catch everything every run. I ran this a ton of times to find everything I did and it seems to be passing now consistently. You could see more crop up in the future occasionally. If that happens they should just be documented as issues and addressed. For other PRs they shouldn't be blocking since a retry of the action will likely get it to pass. |
|
Hi @josh-pritchard, do you have any update on this? |
|
Apologies for the long delay, I will make sure get this cleaned up for you. |
|
Awesome! And don't worry, open-source contributions are voluntary, and I was often enough in the situation myself where other things were more important, so some tickets took really long. |
|
Hi, I have just started using PTerm and loving it but ran into the Data Race errors when running my tests. Is this PR dead or do you have an ETA on when it might get merged? |
|
Hello, any news for a potential fix on this ? |
To resolve #439
There was a very large list of data race conditions when running against the race detector. I have no doubt some of these were causing some inconsistent behavior. I did not change any of the component APIs. I only made a minor change to it in the pterm file (see comments for details). This PR will allow most components to now be used in projects that run tests with race detection on. For now the interactive components that leverage
atomicgo.dev/keyboardstill have race conditions because of that library & I have disabled the race detector for them. It will either need to be switched out or contributed to in order to resolve this. Looking at the lib its not something that I think impacts functionality but it is restrictive when trying to use the project in others that leverage the race detector.Let me know what you think!