Refine usage of defer in config

[arcanericky]

• All tests passed. If this feature is not already covered by the tests, I added new tests.
• All static analysis checks passed.
• This pull request is on the dev branch.
• I used gofmt for formatting the code before submitting the pull request.

---

Refine atomic config command changes in common/commands/ConfigCommand.Run(), int #367, and also mentioned in #352.

Change Details

• The unlock and debug logging are more clean (IMO) when performed in an anonymous function rather than stacking defer statements when possible. Note that I've accounted for defer executing in LIFO order.
• The defer of the anonymous functon containing the lockFile.Unlock() gets executed after the return but before the function returns to the caller (https://go.dev/ref/spec#Defer_statements). This means the setting and check for e not being nil and setting err have no effect on what is returned as might be the intention. See below for more. This change refines the code while leaving the current behavior in place because I know a lot of work went into shoring up the file lock race conditions. If returning an error from lockFile.Unlock() is the intention, more invasive changes must be made, and this isn't the only usage of this pattern (again, see below).
• My gofmt that's part of 1.17.11 really wants to reorganize the imports so I left those changes in since the README explicity states to use gofmt.
• Fixed an extra / in the comment as mentioned in Fixing config file lock mechanism #352 (comment).

For bullet point number 2, I see this pattern scattered throughout the code base:

defer func() {
  e := funcThatCanReturnError()
  if err == nil {
    err = e
  }
}()

Where it is expected that when the surrounding function returns err, could contain the err value set in the defer. This is not correct. You can observe the true behavior by running the following code which I've also placed in the Go Playground at https://go.dev/play/p/gOQB-jPRu4B:

func deferTest() error {
  err := errors.New("top")
  defer func() { err = errors.New("defer") }()
  return err
}

func deferPatternTest() error {
  var err error = nil
  defer func() {
    e := func() error { return errors.New("defer") }()
    if err == nil {
      err = e
    }
  }()
  return err
}

func main() {
  fmt.Println(deferTest())
  fmt.Println(deferPatternTest())
}

which outputs

$ go run deferdemo.go 
top
<nil>

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[arcanericky]

I have read the CLA Document and I hereby sign the CLA

[Or-Geva]

@arcanericky, thanks for your great investigation. It's much appreciated!.
Regarding your bullet point number 2, I believe the right fix will require naming the return value so that the error catch would work.
I've implemented it on the playground demo here.

[arcanericky]

Yes, your solution works great and is simpler than I thought it would be. I'm sure it was overlooked here because in at least a few locations I see where your solution is coded and avoids the problem, though I have found a few more "opportunities".

In the case of ConfigCommand.Run(), more changes were required because the results of function calls are immediately returned. This playground demo matches the code in ConfigCommand.Run() a bit more. My fix is to assign the result of the function calls to err and return at the end. For readability, I like removing err from the return statements. I've pushed my changes with these additions.

[arcanericky]

It's important when you review this to keep in mind the original intention may be to have the return values from the function calls in the switch override any error that may occur with the lockFile.Unlock(). Being an ourside observer, it's tough for me to tell with no comments here.

If this is the case, you should dismiss this PR or have me withdraw it.

[arcanericky]

I've reverted to allowing the function calls in the switch to override any setting of err in the defer. I refuse to be responsible for breaking current behavior (except the origin defer problem), intended or not.

[sverdlov93]

Hi, @arcanericky.
Agree with the first bullet, adjacent defers should be indeed collected together inside an anonymous function.
Regarding the second bullet, the reason behind this order is to avoid deadlock issues.
If somehow the lock would be created and then some error would happen, we must add the unlock function to our defer calls before throwing the error.
Combining the previous order with the (err error) convention will achieve both goals here.

I created a new PR with the above improvements on all of the appearances of the unlock function that exists on the entire project with an explanatory comment.
Also, I added the unlock command to the lock function Callback so it won't be accidentally missed in the future.
cc @eyalbe4

[arcanericky]

Your PR looks good @sverdlov93. One of you can close this PR or I will shut it down at your request.

[sverdlov93]

@arcanericky Thanks again for your contribution and your feedback!
Once my PR will be reviewed I will merge it and close that one.