-
Notifications
You must be signed in to change notification settings - Fork 313
feature(rule): add rule time-after-leak #1273
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
Conversation
|
Hi @chavacava thank you for the PR. I'm wondering if we really should introduce this rule, since go versions below 1.23 are already out of their life cycle. I do however like the idea of the improvements in the code you made. |
denisvmedia
left a comment
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.
LGTM (with a question about linting go projects with go version below 1.23)
|
I share @denisvmedia point of view, with one remark anyway: I have read something yesterday about the fact the bug was only partially fixed for Go 1.23+ Meaning it's way better now, but it still leaks somehow in some circumstances. I will have to find back where it was. |
|
Found it back. https://dev.to/iw4p/how-timeafter-can-cause-memory-leaks-in-go-and-how-to-fix-them-1kg3 Now,I'm doubting about the fact we can trust this article. I mean I checked almost viable resources and found nothing about it. |
| Confidence: 0.8, | ||
| Node: call.Fun, | ||
| Category: lint.FailureCategoryBadPractice, | ||
| Failure: "the underlying goroutine of time.After() is not garbage-collected until timer expiration, prefer NewTimer+Timer.Stop", |
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.
In this error message, you could mention one solution is also to upgrade to Go 1.23+
Or the opposite that this error exists only for version prior 1.23
The same way use-any mentions any can be used with Go 1.18+
https://github.com/mgechev/revive/blob/master/rule%2Fuse_any.go#L50
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.
Here I would like to suggest using a message like the one used for omitzero in structtag rule
Line 330 in 57ed899
| return `prior Go 1.24, option "omitzero" is unsupported`, false |
prior Go 1.24, option "omitzero" is unsupported
| | [`redundant-build-tag`](./RULES_DESCRIPTIONS.md#redundant-build-tag) | n/a | Warns about redundant `// +build` comment lines | no | no | | ||
| | [`use-errors-new`](./RULES_DESCRIPTIONS.md#use-errors-new) | n/a | Spots calls to `fmt.Errorf` that can be replaced by `errors.New` | no | no | | ||
| | [`redundant-test-main-exit`](./RULES_DESCRIPTIONS.md#redundant-test-main-exit) | n/a | Suggests removing `Exit` call in `TestMain` function for test files | no | no | | ||
| | [`time-after-leak`](./RULES_DESCRIPTIONS.md#time-after-leak) | n/a | Suggests replacing `time.After` with `time.NewTimer` to prevent goroutine leak | no | no | |
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 mention here it's for older Go versions, or something like
| | [`time-after-leak`](./RULES_DESCRIPTIONS.md#time-after-leak) | n/a | Suggests replacing `time.After` with `time.NewTimer` to prevent goroutine leak | no | no | | |
| | [`time-after-leak`](./RULES_DESCRIPTIONS.md#time-after-leak) | n/a | Suggests replacing `time.After` with `time.NewTimer` to prevent goroutine leak, if needed | no | no | |
| | [`time-after-leak`](./RULES_DESCRIPTIONS.md#time-after-leak) | n/a | Suggests replacing `time.After` with `time.NewTimer` to prevent goroutine leak | no | no | | |
| | [`time-after-leak`](./RULES_DESCRIPTIONS.md#time-after-leak) | n/a | Suggests replacing `time.After` with `time.NewTimer` to prevent goroutine leak for old Go versions | no | no | |
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.
But maybe, one interesting thing could be to add a column to this table to mention the Go version specification
Something like < 1.23 here, and > 1.18 for use-any, and others.
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.
The following issue was opened
| [rule.superfluous-else] | ||
| arguments = ["preserveScope"] | ||
| ``` | ||
| ## time-after-leak |
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.
Please add a new line before:
| ## time-after-leak | |
| ## time-after-leak |
|
|
||
| _Description_: This rule warns on temporary goroutine leak when using `time.After` in select statements. In Go versions before 1.23, [the garbage collector does not recover the underlying thread until the timer fires](https://pkg.go.dev/time#After). | ||
|
|
||
| _Configuration_: N/A |
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 propose adding a note similar to the ones in range-val-address and range-val-in-closure:
| _Configuration_: N/A | |
| _Configuration_: N/A | |
| _Note_: This rule is irrelevant for Go 1.23+. |
|
Closing. Niche rule for old Go version |
This PR adds a new rule:
time-after-leakThe rule spots the known problem of temporary leak of goroutines underlying
time.After()As described by the documentation of
time.After(), version 1.23 of Go fixed the problem thus the rule skips files of packages with a Go version equal or higher than 1.23.The PR also introduces
lint.IsAtLeastGoVersionto factor-out the common code oflint.IsAtLeastGo1*