From 9f6ef74adc92120e28da864568613536fd9ce3bf Mon Sep 17 00:00:00 2001
From: Alex Sharov <AskAlexSharov@gmail.com>
Date: Fri, 16 Jul 2021 20:23:54 +0700
Subject: [PATCH] Linter: to check tx.Rollback() by ruleguard (#2383)
---
.golangci.yml | 7 +++
cmd/integration/commands/refetence_db.go | 25 ++-------
cmd/snapshots/debug/debug_test.go | 1 +
cmd/state/commands/check_change_sets.go | 1 +
core/state/state_test.go | 2 +-
ethdb/kv/kv_snapshot.go | 2 +-
ethdb/kv/kv_snapshot_property_test.go | 10 ++--
ethdb/kv/memory_database.go | 2 +-
go.mod | 1 +
go.sum | 4 +-
rules.go | 68 ++++++++++++++++++++++++
cmd/hack/binary-deps/main.go => tools.go | 8 +--
turbo/snapshotsync/headers_snapshot.go | 5 +-
13 files changed, 100 insertions(+), 36 deletions(-)
create mode 100644 rules.go
rename cmd/hack/binary-deps/main.go => tools.go (86%)
diff --git a/.golangci.yml b/.golangci.yml
index 7d49626ca9..f5e601e146 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -14,8 +14,15 @@ linters:
- structcheck
- unused
- varcheck
+ - gocritic
linters-settings:
+ gocritic:
+ enabled-checks:
+ - ruleguard
+ settings:
+ ruleguard:
+ rules: "rules.go"
govet:
disable:
- deepequalerrors
diff --git a/cmd/integration/commands/refetence_db.go b/cmd/integration/commands/refetence_db.go
index f589ee0d4b..41b27fc574 100644
--- a/cmd/integration/commands/refetence_db.go
+++ b/cmd/integration/commands/refetence_db.go
@@ -255,9 +255,7 @@ func fToMdbx(ctx context.Context, to string) error {
if err1 != nil {
return err1
}
- defer func() {
- dstTx.Rollback()
- }()
+ defer dstTx.Rollback()
commitEvery := time.NewTicker(5 * time.Second)
defer commitEvery.Stop()
@@ -341,14 +339,6 @@ MainLoop:
if err != nil {
return err
}
- dstTx, err = dst.BeginRw(ctx)
- if err != nil {
- return err
- }
- err = dstTx.Commit()
- if err != nil {
- return err
- }
return nil
}
@@ -370,9 +360,7 @@ func kv2kv(ctx context.Context, src, dst ethdb.RwKV) error {
if err1 != nil {
return err1
}
- defer func() {
- dstTx.Rollback()
- }()
+ defer dstTx.Rollback()
commitEvery := time.NewTicker(30 * time.Second)
defer commitEvery.Stop()
@@ -419,6 +407,7 @@ func kv2kv(ctx context.Context, src, dst ethdb.RwKV) error {
if err != nil {
return err
}
+ defer dstTx.Rollback()
c, err = dstTx.RwCursor(name)
if err != nil {
return err
@@ -442,14 +431,6 @@ func kv2kv(ctx context.Context, src, dst ethdb.RwKV) error {
if err != nil {
return err
}
- dstTx, err = dst.BeginRw(ctx)
- if err != nil {
- return err
- }
- err = dstTx.Commit()
- if err != nil {
- return err
- }
srcTx.Rollback()
log.Info("done")
return nil
diff --git a/cmd/snapshots/debug/debug_test.go b/cmd/snapshots/debug/debug_test.go
index 06050d2320..b179226994 100644
--- a/cmd/snapshots/debug/debug_test.go
+++ b/cmd/snapshots/debug/debug_test.go
@@ -168,6 +168,7 @@ func TestMatreshkaStream(t *testing.T) {
if err != nil {
t.Fatal(err, currentBlock)
}
+ defer tx.Rollback()
dr := time.Since(ttt)
fmt.Println(currentBlock, "finished", "acc-", accDiffLen, "st-", stDiffLen, "codes - ", codesDiffLen, "all -", time.Since(tt), "chunk - ", dr, "blocks/s", 10000/dr.Seconds())
diff --git a/cmd/state/commands/check_change_sets.go b/cmd/state/commands/check_change_sets.go
index d64cab776f..8ac516beef 100644
--- a/cmd/state/commands/check_change_sets.go
+++ b/cmd/state/commands/check_change_sets.go
@@ -230,6 +230,7 @@ func CheckChangeSets(genesis *core.Genesis, blockNum uint64, chaindata string, h
if err != nil {
return err
}
+ defer rwtx.Rollback()
historyTx.Rollback()
historyTx, err = historyDb.BeginRo(context.Background())
if err != nil {
diff --git a/core/state/state_test.go b/core/state/state_test.go
index 3c11e5b861..aed14f4e32 100644
--- a/core/state/state_test.go
+++ b/core/state/state_test.go
@@ -103,7 +103,7 @@ func (s *StateSuite) TestDump(c *checker.C) {
func (s *StateSuite) SetUpTest(c *checker.C) {
s.kv = kv.NewMemKV()
- tx, err := s.kv.BeginRw(context.Background())
+ tx, err := s.kv.BeginRw(context.Background()) //nolint
if err != nil {
panic(err)
}
diff --git a/ethdb/kv/kv_snapshot.go b/ethdb/kv/kv_snapshot.go
index 4a3f213ef7..94b7c9ba1b 100644
--- a/ethdb/kv/kv_snapshot.go
+++ b/ethdb/kv/kv_snapshot.go
@@ -214,7 +214,7 @@ func (s *SnapshotKV) BeginRo(ctx context.Context) (ethdb.Tx, error) {
}
func (s *SnapshotKV) BeginRw(ctx context.Context) (ethdb.RwTx, error) {
- dbTx, err := s.db.BeginRw(ctx)
+ dbTx, err := s.db.BeginRw(ctx) //nolint
if err != nil {
return nil, err
}
diff --git a/ethdb/kv/kv_snapshot_property_test.go b/ethdb/kv/kv_snapshot_property_test.go
index 002b1438f1..d5138dbf5d 100644
--- a/ethdb/kv/kv_snapshot_property_test.go
+++ b/ethdb/kv/kv_snapshot_property_test.go
@@ -45,6 +45,7 @@ func (m *getPutkvMachine) Init(t *rapid.T) {
txSn, err := m.snKV.BeginRw(context.Background())
require.NoError(t, err)
+ defer txSn.Rollback()
txModel, err := m.modelKV.BeginRw(context.Background())
require.NoError(t, err)
@@ -133,9 +134,9 @@ func (m *getPutkvMachine) Begin(t *rapid.T) {
if m.modelTX != nil && m.snTX != nil {
return
}
- mtx, err := m.modelKV.BeginRw(context.Background())
+ mtx, err := m.modelKV.BeginRw(context.Background()) //nolint
require.NoError(t, err)
- sntx, err := m.snKV.BeginRw(context.Background())
+ sntx, err := m.snKV.BeginRw(context.Background()) //nolint
require.NoError(t, err)
m.modelTX = mtx
m.snTX = sntx
@@ -189,6 +190,7 @@ func (m *getKVMachine) Init(t *rapid.T) {
txSn, err := m.snKV.BeginRw(context.Background())
require.NoError(t, err)
+ defer txSn.Rollback()
txModel, err := m.modelKV.BeginRw(context.Background())
require.NoError(t, err)
@@ -343,9 +345,9 @@ func (m *cursorKVMachine) Begin(t *rapid.T) {
return
}
- mtx, err := m.modelKV.BeginRw(context.Background())
+ mtx, err := m.modelKV.BeginRw(context.Background()) //nolint
require.NoError(t, err)
- sntx, err := m.snKV.BeginRw(context.Background())
+ sntx, err := m.snKV.BeginRw(context.Background()) //nolint
require.NoError(t, err)
m.modelTX = mtx
m.snTX = sntx
diff --git a/ethdb/kv/memory_database.go b/ethdb/kv/memory_database.go
index baed6707fd..35e8a1a0d2 100644
--- a/ethdb/kv/memory_database.go
+++ b/ethdb/kv/memory_database.go
@@ -54,7 +54,7 @@ func NewTestTx(t testing.TB) (ethdb.RwKV, ethdb.RwTx) {
tt.Cleanup(kv.Close)
}
}
- tx, err := kv.BeginRw(context.Background())
+ tx, err := kv.BeginRw(context.Background()) //nolint
if err != nil {
t.Fatal(err)
}
diff --git a/go.mod b/go.mod
index a3b62ecb80..13e789854e 100644
--- a/go.mod
+++ b/go.mod
@@ -49,6 +49,7 @@ require (
github.com/petar/GoLLRB v0.0.0-20190514000832-33fb24c13b99
github.com/prometheus/client_golang v1.9.0
github.com/prometheus/tsdb v0.10.0
+ github.com/quasilyte/go-ruleguard/dsl v0.3.6
github.com/rs/cors v1.8.0
github.com/shirou/gopsutil/v3 v3.21.6
github.com/spf13/cobra v1.1.3
diff --git a/go.sum b/go.sum
index 3025ff78f8..26d5f5b110 100644
--- a/go.sum
+++ b/go.sum
@@ -870,6 +870,8 @@ github.com/prometheus/tsdb v0.6.2-0.20190402121629-4f204dcbc150/go.mod h1:qhTCs0
github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU=
github.com/prometheus/tsdb v0.10.0 h1:If5rVCMTp6W2SiRAQFlbpJNgVlgMEd+U2GZckwK38ic=
github.com/prometheus/tsdb v0.10.0/go.mod h1:oi49uRhEe9dPUTlS3JRZOwJuVi6tmh10QSgwXEyGCt4=
+github.com/quasilyte/go-ruleguard/dsl v0.3.6 h1:W2wnISJifyda0x/RXq15Qjrsu9iOhX2gy4+Ku+owylw=
+github.com/quasilyte/go-ruleguard/dsl v0.3.6/go.mod h1:KeCP03KrjuSO0H1kTuZQCWlQPulDV6YMIXmpQss17rU=
github.com/rcrowley/go-metrics v0.0.0-20181016184325-3113b8401b8a/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4=
github.com/rjeczalik/notify v0.9.1/go.mod h1:rKwnCoCGeuQnwBtTSPL9Dad03Vh2n40ePRrjvIXnJho=
github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=
@@ -988,8 +990,6 @@ github.com/tklauser/numcpus v0.2.2 h1:oyhllyrScuYI6g+h/zUvNXNp1wy7x8qQy3t/piefld
github.com/tklauser/numcpus v0.2.2/go.mod h1:x3qojaO3uyYt0i56EW/VUYs7uBvdl2fkfZFu0T9wgjM=
github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
-github.com/torquem-ch/mdbx-go v0.15.0 h1:HUPA06n7tEAyLXv+rFuWvk01FKEm2MavLD9VVEQqMCs=
-github.com/torquem-ch/mdbx-go v0.15.0/go.mod h1:T2fsoJDVppxfAPTLd1svUgH1kpPmeXdPESmroSHcL1E=
github.com/torquem-ch/mdbx-go v0.16.0 h1:x4dTXDvy1w9MxjlAX6J3/bDqIqu4XQDqTR/u2jMnTiA=
github.com/torquem-ch/mdbx-go v0.16.0/go.mod h1:T2fsoJDVppxfAPTLd1svUgH1kpPmeXdPESmroSHcL1E=
github.com/ttacon/chalk v0.0.0-20160626202418-22c06c80ed31/go.mod h1:onvgF043R+lC5RZ8IT9rBXDaEDnpnw/Cl+HFiw+v/7Q=
diff --git a/rules.go b/rules.go
new file mode 100644
index 0000000000..ce5d33b535
--- /dev/null
+++ b/rules.go
@@ -0,0 +1,68 @@
+// +build gorules
+
+package gorules
+
+// https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module
+import (
+ "github.com/quasilyte/go-ruleguard/dsl"
+ //quasilyterules "github.com/quasilyte/ruleguard-rules-test"
+)
+
+func init() {
+ //dsl.ImportRules("qrules", quasilyterules.Bundle)
+}
+
+func txDeferRollback(m dsl.Matcher) {
+ // Common pattern for long-living transactions:
+ // tx, err := db.Begin()
+ // if err != nil {
+ // return err
+ // }
+ // defer tx.Rollback()
+ //
+ // ... code which uses database in transaction
+ //
+ // err := tx.Commit()
+ // if err != nil {
+ // return err
+ // }
+
+ m.Match(
+ `$tx, $err := $db.BeginRw($ctx); $chk; $rollback`,
+ `$tx, $err = $db.BeginRw($ctx); $chk; $rollback`,
+ `$tx, $err := $db.Begin($ctx); $chk; $rollback`,
+ `$tx, $err = $db.Begin($ctx); $chk; $rollback`,
+ ).
+ Where(!m["rollback"].Text.Matches(`defer .*\.Rollback()`)).
+ //At(m["unlock"]).
+ Report(`Add "defer $tx.Rollback()" right after transaction creation error check.
+ If you are in the loop - consider use "$db.View" or "$db.Update" or extract whole transaction to function.
+ Without rollback in defer - app can deadlock on error or panic.
+ Rules are in ./rules.go file.
+ `)
+
+}
+
+func mismatchingUnlock(m dsl.Matcher) {
+ // By default, an entire match position is used as a location.
+ // This can be changed by the At() method that binds the location
+ // to the provided named submatch.
+ //
+ // In the rules below text editor would get mismatching method
+ // name locations:
+ //
+ // defer mu.RUnlock()
+ // ^^^^^^^
+
+ m.Match(`$mu.Lock(); defer $mu.$unlock()`).
+ Where(m["unlock"].Text == "RUnlock").
+ At(m["unlock"]).
+ Report(`maybe $mu.Unlock() was intended?
+ Rules are in ./rules.go file.`)
+
+ m.Match(`$mu.RLock(); defer $mu.$unlock()`).
+ Where(m["unlock"].Text == "Unlock").
+ At(m["unlock"]).
+ Report(`maybe $mu.RUnlock() was intended?
+ Rules are in ./rules.go file.`)
+}
diff --git a/cmd/hack/binary-deps/main.go b/tools.go
similarity index 86%
rename from cmd/hack/binary-deps/main.go
rename to tools.go
index e1560d5717..0eeaaecf42 100644
--- a/cmd/hack/binary-deps/main.go
+++ b/tools.go
@@ -1,5 +1,9 @@
-// +build trick_go_mod_tidy
+// +build tools
+package tools
+
+// https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module
+//
// This module is just a hack for 'go mod tidy' command
//
// Problem is - 'go mod tidy' removes from go.mod file lines if you don't use them in source code
@@ -12,8 +16,6 @@
// build tag 'trick_go_mod_tidy' - is used to hide warnings of IDEA (because we can't import `main` packages in go)
-package main
-
import (
_ "github.com/fjl/gencodec"
_ "github.com/kevinburke/go-bindata"
diff --git a/turbo/snapshotsync/headers_snapshot.go b/turbo/snapshotsync/headers_snapshot.go
index 320a7e1394..6bd0dd8815 100644
--- a/turbo/snapshotsync/headers_snapshot.go
+++ b/turbo/snapshotsync/headers_snapshot.go
@@ -4,14 +4,15 @@ import (
"context"
"errors"
"fmt"
+ "os"
+ "time"
+
"github.com/ledgerwatch/erigon/common"
"github.com/ledgerwatch/erigon/common/dbutils"
"github.com/ledgerwatch/erigon/core/rawdb"
"github.com/ledgerwatch/erigon/ethdb"
"github.com/ledgerwatch/erigon/ethdb/kv"
"github.com/ledgerwatch/erigon/log"
- "os"
- "time"
)
func CreateHeadersSnapshot(ctx context.Context, readTX ethdb.Tx, toBlock uint64, snapshotPath string) error {
--
GitLab