-
Notifications
You must be signed in to change notification settings - Fork 44
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: nil client causes panic #408
Conversation
Signed-off-by: Giovanni Liva <this_that@hotmail.it>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
func (p *Provider) setStatus(status of.State) { | ||
p.mtx.Lock() | ||
defer p.mtx.Unlock() | ||
p.status = status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume this is atomic and therefore locking doesn't help? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this might be needed actually, to prevent interleaving threads checking it during init.
I think I thread isn't blocked by a lock it already owns, right? So I guess you might want to lock in Init() and well as here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do need it, yes - if you execute the tests with the -race
flag enabled, a data race error occurs, due to p.status
being set in the goroutine created within the Init()
function, while being accessed via the Status()
function in the test
This PR
related to: open-feature/go-sdk#238