lint: add new deferunlockcheck pass #107577

Santamaura · 2023-07-25T20:37:05Z

This commit adds a new pass which checks for
...Unlock() expressions without defer which
could result in a lock being held indefinitely.

Resolves #105366

Release note: None

cockroach-teamcity · 2023-07-25T20:37:15Z

This change is

knz

could you also arrange for the check to be disabled if there's a special // nolint:XXX comment next to an Unlock call to say to disable the linter?

See the other linters - we have // nolint:errwrap, // nolint:errcmp etc.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

Santamaura

The check does account for this 😄 the exclusion rule is // nolint:deferunlock

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

rickystewart

Looks like CI is failing, have a look.

rickystewart

Oh, also, add //pkg/testutils/lint/passes/deferunlockcheck along with the other lint checks in the list in top-level BUILD.bazel as part of nogo.

rickystewart · 2023-07-26T16:06:00Z

Looks like you will have to go into build/bazelutil/nogo_config.json and add a stanza for deferunlockcheck so that we only check first-party code. You can copy one of the existing blocks. Let me know if you have any questions.

Santamaura

added

"deferunlockcheck": {

"only\_files": {

"cockroach/pkg/.\*$": "first-party code",

"cockroach/bazel-out/.\*/bin/pkg/.\*$": "first-party code"

}

},

to the nogo_config.json but build still seems to fail on external code..anything additional needed?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

rickystewart · 2023-07-26T16:27:52Z

What's going on with the backslash?

"only\_files":

Santamaura

Odd it wasn't a typo, it's inserted when copy-pasting

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

rickystewart · 2023-07-26T17:53:57Z

You spelled the name of your linter wrong. It's called deferunlockcall, not deferunlockcheck.

This diff works. Should you copy-paste it, be very careful you do not insert backslashes.

diff --git a/build/bazelutil/nogo_config.json b/build/bazelutil/nogo_config.json
index cddfbc90d5e..b5e9b80b09e 100644
--- a/build/bazelutil/nogo_config.json
+++ b/build/bazelutil/nogo_config.json
@@ -28,6 +28,12 @@
             "_test\\.go$": "tests"
         }
     },
+    "deferunlockcall": {
+        "only_files": {
+            "cockroach/pkg/.*$": "first-party code",
+            "cockroach/bazel-out/.*/bin/pkg/.*$": "first-party code"
+        }
+    },
     "errcheck": {
         "exclude_files": {
             "pkg/.*\\.eg\\.go$": "generated code",

rickystewart · 2023-07-26T17:55:17Z

It looks like you use deferunlockcheck in most of the places besides the actual name of the Analyzer. Is that intentional? You may find it easier just to update the Analyzer name to deferunlockcheck if that's what you want it to be called.

rickystewart · 2023-07-26T17:57:47Z

Oh, and the PR calls it deferunlocktest. :P It will probably be much less confusing for everyone if we select one name.

Santamaura

Yeah that was the problem thanks! In this scenario should I nolint the current offenders to pass CI?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

rickystewart · 2023-07-26T19:12:02Z

In this scenario should I nolint the current offenders to pass CI?

If you do, at least file bugs so we can keep track of the current offenders. Otherwise they'll just get lost.

Santamaura · 2023-07-26T19:58:38Z

There is a group effort internally right now to address unsafe locks so I will hold off on merging this until that work progresses since it will resolve the problems this linter is picking up anyways.

Santamaura · 2023-08-07T20:08:09Z

@rickystewart I have PRs that are addressing #107946 but there will still be many manual Unlocks which this linter will catch, is there a way to set the linter such that it would only capture new cases or is the only way to nolint all the remaining safe cases?

rickystewart · 2023-08-07T20:10:31Z

@rickystewart I have PRs that are addressing #107946 but there will still be many manual Unlocks which this linter will catch, is there a way to set the linter such that it would only capture new cases or is the only way to nolint all the remaining safe cases?

You want to nolint all the remaining cases. Otherwise it becomes impossible to lint unless you perform a diff between the commit in question and the commit before this one, which is very confusing -- we don't want lints depending on git history in some way.

Santamaura · 2023-08-24T20:27:00Z

Made the "smarter" linter but still doing some testing. Need to update the nogo_config once done with testing.

Santamaura · 2023-08-28T21:31:16Z

OK this should be RFAL

Santamaura · 2023-08-29T01:27:33Z

Should also mention my team wants me to timebox any more time spent on this linter since it's not really obs inf domain so if this iteration ain't it then I'll probably need to hand it off to dev inf.

stevendanna

Thanks for taking time to address the concerns around this linter.

For me, the inclusion of the "close enough" exception makes this something worth trying.

I'm not convinced we need to report an error on for/range statements. but can live with it if y'all feel it is important.

I briefly read over the linter code, but it should probably have some review from someone more familiar with the relevant APIs.

Santamaura · 2023-08-31T15:53:09Z

Not too sure who to get a review from maybe @renatolabs since you're familiar with the analyzer apis?

rickystewart · 2023-08-31T18:16:20Z

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go

+// shouldReportNonLinearControlFlow checks if locks has a *LockInfo without a matching unlock
+// and if the node has a nolint comment near it. If it finds both it returns true.
+func (lt *LockTracker) shouldReportNonLinearControlFlow(node ast.Node) bool {
+	hasNoLintComment := passesutil.HasNolintComment(lt.pass, node, noLintName)


Seems like you can immediately if hasNoLintComment { return false }, right? We will always return false in this case with the way this function is currently written.

rickystewart · 2023-08-31T18:21:45Z

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go

+}
+
+var (
+	noLintName      = "deferunlock"


Nit: noLintName and maxLineDistance should be const, not var.

rickystewart · 2023-08-31T18:29:05Z

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go

+			if lockDistance > maxLineDistance && !passesutil.HasNolintComment(lt.pass, node, noLintName) {
+				lt.issues = append(lt.issues, analysis.Diagnostic{
+					Pos:     unlockPos,
+					Message: fmt.Sprintf("{RW}Mutex {R}Unlock is >%d lines away from matching {R}Lock", maxLineDistance),


The error message here (and on line 311) is not great. Note that this is the message that will be displayed to devs when they violate the lint rule. In particular it's unclear what someone is supposed to do to solve the problem. e.g. "Oh, I can't have more than 5 lines between the lock and unlock, so I need to start combining multiple lines of code into one line with semicolons" or something like that.

The error message should succinctly describe what the actual problem is and suggest a fix like "move the Unlock() call to a defer statement immediately after the Lock() call". If you produce something the developer can directly copy-paste (like defer x.y.whatever.Unlock() -- I think you actually already have the pieces you need to make this work), even better.

The error message on line 311 has an additional problem because what is "non-linear control flow"? Why is it a problem if I have non-linear control flow, and how do I fix it?

This commit adds a new pass which checks for `...Unlock()` expressions without `defer` which could result in a lock being held indefinitely. Resolves cockroachdb#105366 Release note: None

for linter to pass This commit adds a number of files to the nogo config for the linter to ignore. These files have manual unlocks that are catagorized as potentially problematic since there are unlocks >10 lines away from the lock. The commit also applies the nolint rule to any remaining manual unlock cases that are <=10 lines away from the lock. Release note: None

This commit udpates the lock linter to work by keeping track of locks and if we find any if conditions, loops, or function calls before finding a matching unlock it will report. It will also report if the matching unlock is >5 lines away from the lock. It will ignore cases where a `nolint:deferunlock` is present. Release note: None

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @renatolabs, and @rickystewart)

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go line 40 at r13 (raw file):

Previously, rickystewart (Ricky Stewart) wrote…

Nit: noLintName and maxLineDistance should be const, not var.

Done

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go line 286 at r13 (raw file):

Previously, rickystewart (Ricky Stewart) wrote…

The error message here (and on line 311) is not great. Note that this is the message that will be displayed to devs when they violate the lint rule. In particular it's unclear what someone is supposed to do to solve the problem. e.g. "Oh, I can't have more than 5 lines between the lock and unlock, so I need to start combining multiple lines of code into one line with semicolons" or something like that.

The error message should succinctly describe what the actual problem is and suggest a fix like "move the Unlock() call to a defer statement immediately after the Lock() call". If you produce something the developer can directly copy-paste (like defer x.y.whatever.Unlock() -- I think you actually already have the pieces you need to make this work), even better.

The error message on line 311 has an additional problem because what is "non-linear control flow"? Why is it a problem if I have non-linear control flow, and how do I fix it?

Thanks for the feedback, I made the error messages a bit simpler and added what the dev can do to fix it.

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go line 298 at r13 (raw file):

Previously, rickystewart (Ricky Stewart) wrote…

Seems like you can immediately if hasNoLintComment { return false }, right? We will always return false in this case with the way this function is currently written.

Agree, done.

Santamaura · 2023-09-12T14:06:46Z

TFTRs!

bors r+

craig · 2023-09-12T15:26:52Z

Build succeeded:

Bazel Essential CI (Cockroach)

As part of cockroachdb#107577 an exclusion was added to replica_raft.go. After this PR this exclusion is no longer necessary. Part of: cockroachdb#105366 Release note: None

Santamaura requested a review from a team as a code owner July 25, 2023 20:37

Santamaura requested review from a team and ericharmeling and removed request for a team July 25, 2023 20:38

Santamaura force-pushed the deferunlocktest branch from 8e35304 to 7e11ceb Compare July 25, 2023 20:44

knz reviewed Jul 26, 2023

View reviewed changes

Santamaura force-pushed the deferunlocktest branch from 7e11ceb to 4d41f2b Compare July 26, 2023 14:39

Santamaura commented Jul 26, 2023

View reviewed changes

rickystewart approved these changes Jul 26, 2023

View reviewed changes

rickystewart reviewed Jul 26, 2023

View reviewed changes

Santamaura force-pushed the deferunlocktest branch from 4d41f2b to fe62ff0 Compare July 26, 2023 16:01

Santamaura commented Jul 26, 2023

View reviewed changes

Santamaura changed the title ~~lint: add new deferunlocktest pass~~ lint: add new deferunlockcheck pass Jul 26, 2023

Santamaura commented Jul 26, 2023

View reviewed changes

Santamaura force-pushed the deferunlocktest branch from fe62ff0 to c13bf29 Compare July 26, 2023 19:55

Santamaura mentioned this pull request Aug 1, 2023

Assess and fix potential unsafe locks #107946

Closed

Santamaura force-pushed the deferunlocktest branch 3 times, most recently from 0dc79ce to 3904acc Compare August 11, 2023 14:39

Santamaura force-pushed the deferunlocktest branch 5 times, most recently from fb21941 to 70f1b24 Compare August 28, 2023 20:32

Santamaura requested a review from renatolabs August 28, 2023 21:31

stevendanna reviewed Aug 30, 2023

View reviewed changes

rickystewart requested changes Aug 31, 2023

View reviewed changes

Santamaura added 3 commits September 5, 2023 10:31

lint: add new deferunlocktest pass

f8cf42e

This commit adds a new pass which checks for `...Unlock()` expressions without `defer` which could result in a lock being held indefinitely. Resolves cockroachdb#105366 Release note: None

Santamaura force-pushed the deferunlocktest branch from 70f1b24 to d80a9f5 Compare September 5, 2023 14:31

Santamaura commented Sep 5, 2023

View reviewed changes

Santamaura requested a review from rickystewart September 5, 2023 14:35

rickystewart approved these changes Sep 11, 2023

View reviewed changes

craig bot merged commit b99bcd9 into cockroachdb:master Sep 12, 2023

Santamaura deleted the deferunlocktest branch September 12, 2023 15:35

andrewbaptist mentioned this pull request Apr 16, 2024

gossip: remove race on client peer id #122440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: add new deferunlockcheck pass #107577

lint: add new deferunlockcheck pass #107577

Santamaura commented Jul 25, 2023

cockroach-teamcity commented Jul 25, 2023

knz left a comment

Santamaura left a comment

rickystewart left a comment

rickystewart left a comment

rickystewart commented Jul 26, 2023

Santamaura left a comment

rickystewart commented Jul 26, 2023

Santamaura left a comment

rickystewart commented Jul 26, 2023

rickystewart commented Jul 26, 2023

rickystewart commented Jul 26, 2023

Santamaura left a comment

rickystewart commented Jul 26, 2023

Santamaura commented Jul 26, 2023

Santamaura commented Aug 7, 2023

rickystewart commented Aug 7, 2023

Santamaura commented Aug 24, 2023

Santamaura commented Aug 28, 2023

Santamaura commented Aug 29, 2023

stevendanna left a comment

Santamaura commented Aug 31, 2023 •

edited

Loading

rickystewart Aug 31, 2023

rickystewart Aug 31, 2023

rickystewart Aug 31, 2023 •

edited

Loading

Santamaura left a comment

Santamaura commented Sep 12, 2023

craig bot commented Sep 12, 2023

lint: add new deferunlockcheck pass #107577

lint: add new deferunlockcheck pass #107577

Conversation

Santamaura commented Jul 25, 2023

cockroach-teamcity commented Jul 25, 2023

knz left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

rickystewart left a comment

Choose a reason for hiding this comment

rickystewart left a comment

Choose a reason for hiding this comment

rickystewart commented Jul 26, 2023

Santamaura left a comment

Choose a reason for hiding this comment

rickystewart commented Jul 26, 2023

Santamaura left a comment

Choose a reason for hiding this comment

rickystewart commented Jul 26, 2023

rickystewart commented Jul 26, 2023

rickystewart commented Jul 26, 2023

Santamaura left a comment

Choose a reason for hiding this comment

rickystewart commented Jul 26, 2023

Santamaura commented Jul 26, 2023

Santamaura commented Aug 7, 2023

rickystewart commented Aug 7, 2023

Santamaura commented Aug 24, 2023

Santamaura commented Aug 28, 2023

Santamaura commented Aug 29, 2023

stevendanna left a comment

Choose a reason for hiding this comment

Santamaura commented Aug 31, 2023 • edited Loading

rickystewart Aug 31, 2023

Choose a reason for hiding this comment

rickystewart Aug 31, 2023

Choose a reason for hiding this comment

rickystewart Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

Santamaura commented Sep 12, 2023

craig bot commented Sep 12, 2023

Santamaura commented Aug 31, 2023 •

edited

Loading

rickystewart Aug 31, 2023 •

edited

Loading