Failures in deferred calls not captured

[stmcginnis]

What steps did you take and what happened:

Great suggestion from Stefan in #4499 (comment)

We're probably doing this all across the code base. But I'm a bit concerned about just ignoring errors in those cases. I know
it's not that critical in tests but let's discuss it :)

Can you take a look at: https://github.com/thanos-io/thanos/blob/main/docs/contributing/coding-style-guide.md#reliability

WDYT about the pattern they are using with defer runutil.CloseWithErrCapture(&err, f, "close file") ?

I think it would be a good idea to use something like that to be able to capture failure information in deferred calls.

What did you expect to happen:

It would be great to capture failure information rather than silently swallowing problems.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

• Cluster-api version:
• Minikube/KIND version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

/kind bug

/assign

[vincepri]

Doesn't the linter complain if the error is ignored?

[sbueringer]

@vincepri No, at least not yet. Although I'm not sure if there's a linter we could enable via golangci-lint

Errors in defer calls are pretty frequently ignored (not sure why), especially for things like file.Close.

Hm strange I think errcheck should actually detect those issue. Found a few rejcted issues in the linter repo of folks wo wanted to disable the linter on defer calls:

• ignore defer calls kisielk/errcheck#55
• errcheck should not detect "defer" with error  kisielk/errcheck#101

The search is not ideal, but even at a first glance we have a few occurences of ignored errors in defer Close statements: https://github.com/kubernetes-sigs/cluster-api/search?q=defer+Close

[vincepri]

/milestone Next
/priority backlog
/help

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/milestone Next
/priority backlog
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sbueringer]

Doesn't the linter complain if the error is ignored?

Apparently we have a linter for it. gosec finding G307 detects some of these cases. The findings we have there seem reasonable: G307: Deferring unsafe method "Close" on type "*os.File (9x). I think if we want to address those we should probably do it similarly as Thanos here: https://github.com/thanos-io/thanos/blob/main/docs/contributing/coding-style-guide.md#defers-dont-forget-to-check-returned-errors (CloseWithErrCapture )

(@vincepri)

[vincepri]

@sbueringer Is there any action items we have as part of this issue? If we can rework it to be more specific and provide a potential solution, we can also mark it as good-first-issue

[sbueringer]

@vincepri In my opinion we have two options depending on if we want to fix G307: Deferring unsafe method "Close" on type "*os.File:

1. if not: we should move the exclude of G307 above # The following are being worked on to remove their exclusion. This list should be reduced or go away all together over time. in .golangci.yml

2. if yes: I would propose to implement it as described in the Thanos style guide by implementing a CloseWithErrCapture util func which we can then use like this: defer runutil.CloseWithErrCapture(&err, f, "close file")

I think option 1 might not be worth it to flag it as good-first-issue as it's just moving one line in the linter config. Option 2 would be good for good-first-issue. We just should decide first :)

[vincepri]

I'm ok to leave that as-is for now, file closure failure are usually a sign for other things failing way earlier, although if we want to fix it that's also fine

[sbueringer]

Same for me. Let's see if @CecileRobertMichon or @fabriziopandini have a strong opinion on that. Otherwise I would adjust the linter config and close the issue.

[fabriziopandini]

+1 to leave as it is