fix: Make rendering safe to use concurrently - #1004
Conversation
I found out when I started using ytt as a library, rather than a CLI, that race tests were failing. I tracked it back down to ytt itself. There were two locations that were using global variables that weren't protected by some sort of syncronization. I made some small changes, along with some tests to check for it as well. Signed-off-by: Taylor Thomas <taylor.thomas@akuity.io>
| func (e *CompiledTemplate) Eval(thread *starlark.Thread, loader CompiledTemplateLoader) ( | ||
| starlark.StringDict, interface{}, error) { | ||
|
|
||
| starlark.StringDict, interface{}, error, |
There was a problem hiding this comment.
All of these came from the auto formatter running. If they're bothersome, I can revert them
| // happens whenever ytt is embedded as a Go module and more than one render is | ||
| // in flight. The values are process-wide by nature and never change, so this is | ||
| // the best way to set it | ||
| func init() { |
There was a problem hiding this comment.
This was the least important of the two races as it just was re-setting the same value on each call, but it was always the same value. So this is mostly to not fail when running go test -race
| func NewInstructionSet() *InstructionSet { | ||
| globalInsSetID++ | ||
| uniqueID := globalInsSetID | ||
| uniqueID := globalInsSetID.Add(1) |
There was a problem hiding this comment.
This race was the one that could cause an actual bug. Not sure how it would manifest in the instruction op (since this is my first time in the ytt codebase), but you could have races of this variable and an InstructionSet would end up with ops containing different IDs
Signed-off-by: Taylor Thomas <taylor.thomas@akuity.io>
|
The lint failures are with existing code. I fixed them in an additional commit, but if you don't want them, I can pop it off |
2 participants
I found out when I started using ytt as a library, rather than a CLI, that race tests were failing. I tracked it back down to ytt itself. There were two locations that were using global variables that weren't protected by some sort of syncronization. I made some small changes, along with some tests to check for it as well.