Replace Viper #1263
Replace Viper #1263
Conversation
Also moves the pidlockfile implementation to its own package
| i.rwLock.RLock() | ||
| defer i.rwLock.RUnlock() |
There was a problem hiding this comment.
I think we still need this, otherwise we could encounter a panic attempting to read from a map that is currently being written to
There was a problem hiding this comment.
How? The only way that that could happen is if a subroutine is writing to i.data, in which case it would be more appropriate to use a mutex. But I think that mutex probably makes more sense when writing than when reading.
There was a problem hiding this comment.
The scenario you describe is also the one I'm thinking of. It is unlikely to happen in the same instance of the state tool but still possible I think. The rwLock is a mutex which is used in both Set and Get
There was a problem hiding this comment.
A small aside: In almost all cases a RWLock costs more than the optimization it provides. In other words, it requires quite a bit of contention before it's worth using.
Update lockfile name Remove unnecessary function
| i.rwLock.RLock() | ||
| defer i.rwLock.RUnlock() |
There was a problem hiding this comment.
How? The only way that that could happen is if a subroutine is writing to i.data, in which case it would be more appropriate to use a mutex. But I think that mutex probably makes more sense when writing than when reading.
| i.rwLock.RLock() | ||
| defer i.rwLock.RUnlock() |
There was a problem hiding this comment.
A small aside: In almost all cases a RWLock costs more than the optimization it provides. In other words, it requires quite a bit of contention before it's worth using.
internal/config/instance.go
Outdated
| @@ -231,6 +231,10 @@ func (i *Instance) ReadInConfig() error { | |||
|
|
|||
| err = pl.WaitForLock(5 * time.Second) | |||
| if err != nil { | |||
| lockedErr := &lockfile.AlreadyLockedError{} | |||
| if errs.Matches(err, &lockedErr) { | |||
| return errs.Wrap(err, "Could not write config as another process appears to be using it") | |||
There was a problem hiding this comment.
Why did you just detect the error type just to wrap it with a regular error? Did you intend to localize this?
There was a problem hiding this comment.
No, importing locale in this package causes an import cycle. The intent here was to add some more context to the error. Do these wrapped messages not bubble up to the user in the same way as the localized errors? If so, I can remove this if it won't be helpful.
There was a problem hiding this comment.
At a glance, I think it makes sense to not return localized errors from this package, and then have handlers provide localized errors.
If that's not acceptable, I suggest that locale is doing too much by affecting persistent application state.
*Edit to add: Sorry to butt in, I was pinged by email and forgot I was only to look over the channel stuff.
There was a problem hiding this comment.
Non localized errors do not bubble up to the user. And the error chain is its own context. I think we can drop this.
internal/osutils/lockfile/pidlock.go
Outdated
| select { | ||
| case <-timer.C: | ||
| return err | ||
| default: | ||
| time.Sleep(100 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
This should work fine - just noting that you had thumbed-up my suggestion to use time.Time.After instead.
There was a problem hiding this comment.
Oh, I missed this... Using a timer requires more detailed usage: golang/go#27169 (comment)
I do think avoiding the channel by using time.Time.After is worthwhile here.
There was a problem hiding this comment.
Whoops, forgot to push the commit where I use time.Now().After() 🤦
internal/config/instance.go
Outdated
| @@ -231,6 +231,10 @@ func (i *Instance) ReadInConfig() error { | |||
|
|
|||
| err = pl.WaitForLock(5 * time.Second) | |||
| if err != nil { | |||
| lockedErr := &lockfile.AlreadyLockedError{} | |||
| if errs.Matches(err, &lockedErr) { | |||
| return errs.Wrap(err, "Could not write config as another process appears to be using it") | |||
There was a problem hiding this comment.
Non localized errors do not bubble up to the user. And the error chain is its own context. I think we can drop this.
https://www.pivotaltracker.com/story/show/176754995