Skip to content

Commit

Permalink
lint: exclude problem files and apply nolint to minor cases in order
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Santamaura committed Sep 5, 2023
1 parent f8cf42e commit 8368312
Show file tree
Hide file tree
Showing 281 changed files with 959 additions and 7 deletions.
85 changes: 85 additions & 0 deletions build/bazelutil/nogo_config.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,91 @@
}
},
"deferunlockcheck": {
"exclude_files": {
"pkg/ccl/backupccl/restore_progress.go": "unlock >10 lines away from lock",
"pkg/ccl/changefeedccl/cdctest/mock_webhook_sink.go": "unlock >10 lines away from lock",
"pkg/ccl/changefeedccl/schemafeed/schema_feed.go": "unlock >10 lines away from lock",
"pkg/ccl/changefeedccl/sink_kafka.go": "unlock >10 lines away from lock",
"pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go": "unlock >10 lines away from lock",
"pkg/cmd/generate-spatial-ref-sys/main.go": "unlock >10 lines away from lock",
"pkg/cmd/roachtest/cluster.go": "unlock >10 lines away from lock",
"pkg/cmd/roachtest/test_runner.go": "unlock >10 lines away from lock",
"pkg/cmd/smithtest/main.go": "unlock >10 lines away from lock",
"pkg/gossip/client.go": "unlock >10 lines away from lock",
"pkg/gossip/gossip.go": "unlock >10 lines away from lock",
"pkg/gossip/server.go": "unlock >10 lines away from lock",
"pkg/gossip/storage_test.go": "unlock >10 lines away from lock",
"pkg/internal/sqlsmith/bulkio.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/kvcoord/txn_coord_sender.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/kvcoord/txn_interceptor_heartbeater_test.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/kvcoord/txn_interceptor_heartbeater.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/kvcoord/txn_lock_gatekeeper.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/kvstreamer/results_buffer_test.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/kvstreamer/streamer.go": "unlock >10 lines away from lock",
"pkg/kv/kvclient/rangecache/range_cache.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/client_merge_test.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/closedts/sidetransport/debug.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/closedts/sidetransport/sender.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/closedts/tracker/tracker_test.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/concurrency/lock_table.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/intentresolver/intent_resolver.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle": "unlock >10 lines away from lock",
"pkg/kv/kvserver/liveness/cache.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/metrics.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/queue.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/raft_log_queue.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/raft_log_truncator.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/raftentry/cache.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/rangefeed/registry.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_app_batch.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_metrics.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_proposal_buf.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_raft.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_raftstorage.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_rangefeed.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_test.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/replica_write.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/scheduler.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/store.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/store_test.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/store_create_replica.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/store_gossip.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/store_raft.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/store_remove_replica.go": "unlock >10 lines away from lock",
"pkg/kv/kvserver/txnwait/queue.go": "unlock >10 lines away from lock",
"pkg/obs/event_exporter.go": "unlock >10 lines away from lock",
"pkg/rpc/clock_offset.go": "unlock >10 lines away from lock",
"pkg/server/status/recorder_test.go": "unlock >10 lines away from lock",
"pkg/server/telemetry/features.go": "unlock >10 lines away from lock",
"pkg/sql/catalog/lease/lease_test.go": "unlock >10 lines away from lock",
"pkg/sql/colflow/routers.go": "unlock >10 lines away from lock",
"pkg/sql/contention/contentionutils/concurrent_buffer_guard.go": "unlock >10 lines away from lock",
"pkg/sql/distsql_running.go": "unlock >10 lines away from lock",
"pkg/sql/distsql_running_test.go": "unlock >10 lines away from lock",
"pkg/sql/flowinfra/flow_registry.go": "unlock >10 lines away from lock",
"pkg/sql/idxusage/local_idx_usage_stats.go": "unlock >10 lines away from lock",
"pkg/sql/pgwire/server.go": "unlock >10 lines away from lock",
"pkg/sql/rowflow/routers.go": "unlock >10 lines away from lock",
"pkg/sql/schema_changer_test.go": "unlock >10 lines away from lock",
"pkg/sql/stats/stats_cache.go": "unlock >10 lines away from lock",
"pkg/sql/tests/rsg_test.go": "unlock >10 lines away from lock",
"pkg/sql/txn_restart_test.go": "unlock >10 lines away from lock",
"pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go": "test file for the linter",
"pkg/testutils/storageutils/mocking.go": "unlock >10 lines away from lock",
"pkg/util/admission/work_queue.go": "unlock >10 lines away from lock",
"pkg/util/circuit/circuitbreaker.go": "unlock >10 lines away from lock",
"pkg/util/metric/hdrhistogram.go": "unlock >10 lines away from lock",
"pkg/util/mon/bytes_usage.go": "unlock >10 lines away from lock",
"pkg/util/syncutil/int_map.go": "unlock >10 lines away from lock",
"pkg/util/syncutil/singleflight/singleflight": "unlock >10 lines away from lock",
"pkg/util/tracing/crdbspan.go": "unlock >10 lines away from lock",
"pkg/util/tracing/span.go": "unlock >10 lines away from lock",
"pkg/workload/histogram/histogram.go": "unlock >10 lines away from lock",
"pkg/workload/schemachange/schemachange.go": "unlock >10 lines away from lock",
"pkg/workload/tpcc/generate.go": "unlock >10 lines away from lock"
},
"only_files": {
"cockroach/pkg/.*$": "first-party code",
"cockroach/bazel-out/.*/bin/pkg/.*$": "first-party code"
Expand Down
8 changes: 8 additions & 0 deletions pkg/acceptance/localcluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,7 @@ func (n *Node) Alive() bool {
func (n *Node) StatusClient(ctx context.Context) serverpb.StatusClient {
n.Lock()
existingClient := n.statusClient
// nolint:deferunlock
n.Unlock()

if existingClient != nil {
Expand Down Expand Up @@ -588,6 +589,7 @@ func (n *Node) startAsyncInnerLocked(ctx context.Context, joins ...string) error
_ = errors.As(waitErr, &execErr)
n.Lock()
n.setNotRunningLocked(execErr)
// nolint:deferunlock
n.Unlock()
}(n.cmd)

Expand Down Expand Up @@ -695,6 +697,7 @@ func (n *Node) waitUntilLive(dur time.Duration) error {
if n.cmd != nil {
pid = n.cmd.Process.Pid
}
// nolint:deferunlock
n.Unlock()
if pid == 0 {
log.Info(ctx, "process already quit")
Expand All @@ -717,12 +720,14 @@ func (n *Node) waitUntilLive(dur time.Duration) error {
if n.Cfg.RPCPort == 0 {
n.Lock()
n.rpcPort = pgURL.Port()
// nolint:deferunlock
n.Unlock()
}

pgURL.Path = n.Cfg.DB
n.Lock()
n.pgURL = pgURL.String()
// nolint:deferunlock
n.Unlock()

var uiURL *url.URL
Expand All @@ -739,6 +744,7 @@ func (n *Node) waitUntilLive(dur time.Duration) error {
// http port is required but isn't initialized yet.
n.Lock()
n.db = makeDB(n.pgURL, n.Cfg.NumWorkers, n.Cfg.DB)
// nolint:deferunlock
n.Unlock()

{
Expand Down Expand Up @@ -769,6 +775,7 @@ func (n *Node) Kill() {
for ok := false; !ok; {
n.Lock()
ok = n.cmd == nil
// nolint:deferunlock
n.Unlock()
}
}
Expand Down Expand Up @@ -804,6 +811,7 @@ func (n *Node) Signal(s os.Signal) {
func (n *Node) Wait() *exec.ExitError {
n.Lock()
ch := n.notRunning
// nolint:deferunlock
n.Unlock()
if ch == nil {
log.Warning(context.Background(), "(*Node).Wait called when node was not running")
Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/auditloggingccl/audit_log_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ func UpdateAuditConfigOnChange(
}
acl.Lock()
acl.Config = config
// nolint:deferunlock
acl.Unlock()
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/ccl/backupccl/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2593,9 +2593,11 @@ func TestBackupRestoreDuringUserDefinedTypeChange(t *testing.T) {
if numTypeChangesStarted == len(tc.queries) {
close(typeChangesStarted)
}
// nolint:deferunlock
mu.Unlock()
<-waitForBackup
} else {
// nolint:deferunlock
mu.Unlock()
}
return nil
Expand Down Expand Up @@ -2624,6 +2626,7 @@ CREATE TYPE d.greeting AS ENUM ('hello', 'howdy', 'hi');
if numTypeChangesFinished == totalQueries {
close(typeChangesFinished)
}
// nolint:deferunlock
mu.Unlock()
}(query, len(tc.queries))
}
Expand Down Expand Up @@ -6205,6 +6208,7 @@ func TestRestoreErrorPropagates(t *testing.T) {

jobsTableKey.Lock()
jobsTableKey.key = tc.ApplicationLayer(0).Codec().TablePrefix(uint32(systemschema.JobsTable.GetID()))
// nolint:deferunlock
jobsTableKey.Unlock()

runner.Exec(t, `SET CLUSTER SETTING jobs.metrics.interval.poll = '30s'`)
Expand Down Expand Up @@ -6387,6 +6391,7 @@ INSERT INTO foo.bar VALUES (110), (210), (310), (410), (510)`)
startingSpan := mkSpan(id1, "/Tenant/10/Table/:id/1", "/Tenant/10/Table/:id/2")
mu.Lock()
require.Equal(t, []string{startingSpan.String()}, mu.exportRequestSpans)
// nolint:deferunlock
mu.Unlock()
resetStateVars()

Expand All @@ -6397,6 +6402,7 @@ INSERT INTO foo.bar VALUES (110), (210), (310), (410), (510)`)
resumeSpan := mkSpan(id1, "/Tenant/10/Table/:id/1/510/0", "/Tenant/10/Table/:id/2")
mu.Lock()
require.Equal(t, []string{startingSpan.String(), resumeSpan.String()}, mu.exportRequestSpans)
// nolint:deferunlock
mu.Unlock()
resetStateVars()

Expand All @@ -6415,6 +6421,7 @@ INSERT INTO foo.bar VALUES (110), (210), (310), (410), (510)`)
}
mu.Lock()
require.Equal(t, expected, mu.exportRequestSpans)
// nolint:deferunlock
mu.Unlock()
resetStateVars()

Expand Down Expand Up @@ -6448,6 +6455,7 @@ INSERT INTO baz.bar VALUES (110, 'a'), (210, 'b'), (310, 'c'), (410, 'd'), (510,
}
mu.Lock()
require.Equal(t, expected, mu.exportRequestSpans)
// nolint:deferunlock
mu.Unlock()
resetStateVars()

Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/backupccl/backupinfo/manifest_handling.go
Original file line number Diff line number Diff line change
Expand Up @@ -1541,6 +1541,7 @@ func GetBackupManifests(
manifests[i] = desc
}
subMem.Shrink(ctx, size)
// nolint:deferunlock
memMu.Unlock()

return err
Expand Down
2 changes: 2 additions & 0 deletions pkg/ccl/backupccl/restore_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ func restore(
introducedSpanFrontier,
targetRestoreSpanSize.Get(&execCtx.ExecCfg().Settings.SV),
progressTracker.useFrontier)
// nolint:deferunlock
progressTracker.mu.Unlock()
if err != nil {
return roachpb.RowCount{}, err
Expand Down Expand Up @@ -2153,6 +2154,7 @@ func insertStats(
mu.completedBatches++
remainingBatches := totalNumBatches - mu.completedBatches
completedBatches := mu.completedBatches
// nolint:deferunlock
mu.Unlock()
if insertStatsProgress.ShouldLog() {
logStatsProgress(remainingBatches, completedBatches)
Expand Down
2 changes: 2 additions & 0 deletions pkg/ccl/backupccl/tenant_backup_nemesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -428,13 +428,15 @@ func (r *randomBackupNemesis) Stop() {
func (r *randomBackupNemesis) TablesToCheck() []string {
r.mu.Lock()
ret := append([]string(nil), r.mu.tablesToCheck...)
// nolint:deferunlock
r.mu.Unlock()
return ret
}

func (r *randomBackupNemesis) addTable(name string) {
r.mu.Lock()
r.mu.tablesToCheck = append(r.mu.tablesToCheck, name)
// nolint:deferunlock
r.mu.Unlock()
}

Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/changefeedccl/cdctest/schema_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ func (r *SchemaRegistry) EncodedAvroToNative(b []byte) (interface{}, error) {

r.mu.Lock()
jsonSchema := r.mu.schemas[id]
// nolint:deferunlock
r.mu.Unlock()
codec, err := goavro.NewCodec(jsonSchema)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions pkg/ccl/changefeedccl/changefeed_processors.go
Original file line number Diff line number Diff line change
Expand Up @@ -1187,6 +1187,7 @@ func (cf *changeFrontier) Start(ctx context.Context) {
cf.metricsID = cf.metrics.mu.id
cf.metrics.mu.id++
sli.RunningCount.Inc(1)
// nolint:deferunlock
cf.metrics.mu.Unlock()

cf.sliMetricsID = cf.sliMetrics.claimId()
Expand Down Expand Up @@ -1229,6 +1230,7 @@ func (cf *changeFrontier) closeMetrics() {
}
delete(cf.metrics.mu.resolved, cf.metricsID)
cf.metricsID = -1
// nolint:deferunlock
cf.metrics.mu.Unlock()

cf.sliMetrics.closeId(cf.sliMetricsID)
Expand Down Expand Up @@ -1381,6 +1383,7 @@ func (cf *changeFrontier) forwardFrontier(resolved jobspb.ResolvedSpan) error {
if cf.metricsID != -1 {
cf.metrics.mu.resolved[cf.metricsID] = newResolved
}
// nolint:deferunlock
cf.metrics.mu.Unlock()

return cf.maybeEmitResolved(newResolved)
Expand Down
3 changes: 3 additions & 0 deletions pkg/ccl/changefeedccl/event_processing.go
Original file line number Diff line number Diff line change
Expand Up @@ -646,13 +646,15 @@ func (c *parallelEventConsumer) incInFlight() {
c.mu.Lock()
c.mu.inFlight++
c.metrics.ParallelConsumerInFlightEvents.Update(int64(c.mu.inFlight))
// nolint:deferunlock
c.mu.Unlock()
}

func (c *parallelEventConsumer) decInFlight() {
c.mu.Lock()
c.mu.inFlight--
notifyFlush := c.mu.waiting && c.mu.inFlight == 0
// nolint:deferunlock
c.mu.Unlock()

// If someone is waiting on a flush, signal to them
Expand Down Expand Up @@ -706,6 +708,7 @@ func (c *parallelEventConsumer) Flush(ctx context.Context) error {
c.mu.Lock()
c.mu.waiting = false
c.mu.flushFrontier = c.spanFrontier.Frontier()
// nolint:deferunlock
c.mu.Unlock()
return nil
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/ccl/changefeedccl/kvevent/blocking_buffer.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ func (b *blockingBuffer) pop() (e Event, ok bool, err error) {
func (b *blockingBuffer) notifyOutOfQuota(canFlush bool) {
b.mu.Lock()
b.mu.canFlush = canFlush
// nolint:deferunlock
b.mu.Unlock()

if canFlush {
Expand All @@ -165,6 +166,7 @@ func (b *blockingBuffer) notifyOutOfQuota(canFlush bool) {
func (b *blockingBuffer) producerBlocked() {
b.mu.Lock()
b.mu.numBlocked++
// nolint:deferunlock
b.mu.Unlock()
}

Expand All @@ -183,6 +185,7 @@ func (b *blockingBuffer) quotaAcquiredAfterWait() {
// Clear out canFlush since we know that producers no longer blocked.
b.mu.canFlush = false
}
// nolint:deferunlock
b.mu.Unlock()
}

Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/changefeedccl/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,7 @@ func MakeMetrics(histogramWindow time.Duration) metric.Struct {
maxBehind = behind
}
}
// nolint:deferunlock
m.mu.Unlock()
return maxBehind.Nanoseconds()
})
Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/changefeedccl/schema_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ func newConfluentSchemaRegistry(
}
schemaRegistrySingletons.cachePerEndpoint[baseURL] = src
}
// nolint:deferunlock
schemaRegistrySingletons.mu.Unlock()

s, err := getAndDeleteParams(u)
Expand Down
3 changes: 3 additions & 0 deletions pkg/ccl/changefeedccl/schemafeed/schema_feed.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ func (tf *schemaFeed) Run(ctx context.Context) error {
func (tf *schemaFeed) primeInitialTableDescs(ctx context.Context) error {
tf.mu.Lock()
initialTableDescTs := tf.mu.highWater
// nolint:deferunlock
tf.mu.Unlock()
var initialDescs []catalog.Descriptor

Expand Down Expand Up @@ -312,6 +313,7 @@ func (tf *schemaFeed) primeInitialTableDescs(ctx context.Context) error {
tbl := desc.(catalog.TableDescriptor)
tf.mu.typeDeps.ingestTable(tbl)
}
// nolint:deferunlock
tf.mu.Unlock()

return tf.ingestDescriptors(ctx, hlc.Timestamp{}, initialTableDescTs, initialDescs, tf.validateDescriptor)
Expand Down Expand Up @@ -510,6 +512,7 @@ func (tf *schemaFeed) pauseOrResumePolling(
func (tf *schemaFeed) highWater() hlc.Timestamp {
tf.mu.Lock()
highWater := tf.mu.highWater
// nolint:deferunlock
tf.mu.Unlock()
return highWater
}
Expand Down
Loading

0 comments on commit 8368312

Please sign in to comment.