Skip to content
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

core, miner, rpc, eth: fix goroutine leaks in tests #24211

Merged
merged 2 commits into from
Jan 21, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions core/blockchain.go
Original file line number Diff line number Diff line change
Expand Up @@ -800,8 +800,12 @@ func (bc *BlockChain) Stop() {

for _, offset := range []uint64{0, 1, TriesInMemory - 1} {
if number := bc.CurrentBlock().NumberU64(); number > offset {
recent := bc.GetBlockByNumber(number - offset)

num := number - offset
recent := bc.GetBlockByNumber(num)
if recent == nil {
log.Error("Failed to get block", num)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Error("Failed to get block", num)
log.Error("Failed to get block", "number", num)

continue
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd rather revert this -- is this path triggered by the tests?
Also, the log format is wrong, would need to be log.Error("Failed to get block", "num", num)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I could tell, the tests didn't trigger this path, so I have reverted this change. If this happens, it indicates that something is seriously wrong, either our blockchain is corrupt, or the database is closed.

log.Info("Writing cached state to disk", "block", recent.Number(), "hash", recent.Hash(), "root", recent.Root())
if err := triedb.Commit(recent.Root(), true, nil); err != nil {
log.Error("Failed to commit recent state trie", "err", err)
Expand Down
19 changes: 10 additions & 9 deletions core/blockchain_repair_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1779,6 +1779,7 @@ func testRepair(t *testing.T, tt *rewindTest, snapshots bool) {
SnapshotLimit: 0, // Disable snapshot by default
}
)
defer engine.Close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to close the FullFaker ethash engine? There is nothing to do in the Close function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it can't be necessary, but doing it is more correct, so I'd rather just keep it htere

if snapshots {
config.SnapshotLimit = 256
config.SnapshotWait = true
Expand Down Expand Up @@ -1836,25 +1837,25 @@ func testRepair(t *testing.T, tt *rewindTest, snapshots bool) {
}
defer db.Close()

chain, err = NewBlockChain(db, nil, params.AllEthashProtocolChanges, engine, vm.Config{}, nil, nil)
newChain, err := NewBlockChain(db, nil, params.AllEthashProtocolChanges, engine, vm.Config{}, nil, nil)
if err != nil {
t.Fatalf("Failed to recreate chain: %v", err)
}
defer chain.Stop()
defer newChain.Stop()

// Iterate over all the remaining blocks and ensure there are no gaps
verifyNoGaps(t, chain, true, canonblocks)
verifyNoGaps(t, chain, false, sideblocks)
verifyCutoff(t, chain, true, canonblocks, tt.expCanonicalBlocks)
verifyCutoff(t, chain, false, sideblocks, tt.expSidechainBlocks)
verifyNoGaps(t, newChain, true, canonblocks)
verifyNoGaps(t, newChain, false, sideblocks)
verifyCutoff(t, newChain, true, canonblocks, tt.expCanonicalBlocks)
verifyCutoff(t, newChain, false, sideblocks, tt.expSidechainBlocks)

if head := chain.CurrentHeader(); head.Number.Uint64() != tt.expHeadHeader {
if head := newChain.CurrentHeader(); head.Number.Uint64() != tt.expHeadHeader {
t.Errorf("Head header mismatch: have %d, want %d", head.Number, tt.expHeadHeader)
}
if head := chain.CurrentFastBlock(); head.NumberU64() != tt.expHeadFastBlock {
if head := newChain.CurrentFastBlock(); head.NumberU64() != tt.expHeadFastBlock {
t.Errorf("Head fast block mismatch: have %d, want %d", head.NumberU64(), tt.expHeadFastBlock)
}
if head := chain.CurrentBlock(); head.NumberU64() != tt.expHeadBlock {
if head := newChain.CurrentBlock(); head.NumberU64() != tt.expHeadBlock {
t.Errorf("Head block mismatch: have %d, want %d", head.NumberU64(), tt.expHeadBlock)
}
if frozen, err := db.(freezer).Ancients(); err != nil {
Expand Down
4 changes: 3 additions & 1 deletion eth/fetcher/block_fetcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ func testSequentialAnnouncements(t *testing.T, light bool) {
hashes, blocks := makeChain(targetBlocks, 0, genesis)

tester := newTester(light)
defer tester.fetcher.Stop()
headerFetcher := tester.makeHeaderFetcher("valid", blocks, -gatherSlack)
bodyFetcher := tester.makeBodyFetcher("valid", blocks, 0)

Expand Down Expand Up @@ -743,7 +744,7 @@ func testInvalidNumberAnnouncement(t *testing.T, light bool) {
badBodyFetcher := tester.makeBodyFetcher("bad", blocks, 0)

imported := make(chan interface{})
announced := make(chan interface{})
announced := make(chan interface{}, 2)
tester.fetcher.importedHook = func(header *types.Header, block *types.Block) {
if light {
if header == nil {
Expand Down Expand Up @@ -806,6 +807,7 @@ func TestEmptyBlockShortCircuit(t *testing.T) {
hashes, blocks := makeChain(32, 0, genesis)

tester := newTester(false)
defer tester.fetcher.Stop()
headerFetcher := tester.makeHeaderFetcher("valid", blocks, -gatherSlack)
bodyFetcher := tester.makeBodyFetcher("valid", blocks, 0)

Expand Down
1 change: 1 addition & 0 deletions graphql/graphql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ func TestBuildSchema(t *testing.T) {
conf := node.DefaultConfig
conf.DataDir = ddir
stack, err := node.New(&conf)
defer stack.Close()
if err != nil {
t.Fatalf("could not create new node: %v", err)
}
Expand Down
8 changes: 4 additions & 4 deletions internal/jsre/jsre_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,20 +83,20 @@ func TestNatto(t *testing.T) {

err := jsre.Exec("test.js")
if err != nil {
t.Errorf("expected no error, got %v", err)
t.Fatalf("expected no error, got %v", err)
}
time.Sleep(100 * time.Millisecond)
val, err := jsre.Run("msg")
if err != nil {
t.Errorf("expected no error, got %v", err)
t.Fatalf("expected no error, got %v", err)
}
if val.ExportType().Kind() != reflect.String {
t.Errorf("expected string value, got %v", val)
t.Fatalf("expected string value, got %v", val)
}
exp := "testMsg"
got := val.ToString().String()
if exp != got {
t.Errorf("expected '%v', got '%v'", exp, got)
t.Fatalf("expected '%v', got '%v'", exp, got)
}
jsre.Stop(false)
}
Expand Down
36 changes: 26 additions & 10 deletions miner/miner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ func (bc *testBlockChain) SubscribeChainHeadEvent(ch chan<- core.ChainHeadEvent)
}

func TestMiner(t *testing.T) {
miner, mux := createMiner(t)
miner, mux, cleanup := createMiner(t)
defer cleanup(false)
miner.Start(common.HexToAddress("0x12345"))
waitForMiningState(t, miner, true)
// Start the downloader
Expand All @@ -107,7 +108,8 @@ func TestMiner(t *testing.T) {
// An initial FailedEvent should allow mining to stop on a subsequent
// downloader StartEvent.
func TestMinerDownloaderFirstFails(t *testing.T) {
miner, mux := createMiner(t)
miner, mux, cleanup := createMiner(t)
defer cleanup(false)
miner.Start(common.HexToAddress("0x12345"))
waitForMiningState(t, miner, true)
// Start the downloader
Expand Down Expand Up @@ -138,8 +140,8 @@ func TestMinerDownloaderFirstFails(t *testing.T) {
}

func TestMinerStartStopAfterDownloaderEvents(t *testing.T) {
miner, mux := createMiner(t)

miner, mux, cleanup := createMiner(t)
defer cleanup(false)
miner.Start(common.HexToAddress("0x12345"))
waitForMiningState(t, miner, true)
// Start the downloader
Expand All @@ -161,7 +163,8 @@ func TestMinerStartStopAfterDownloaderEvents(t *testing.T) {
}

func TestStartWhileDownload(t *testing.T) {
miner, mux := createMiner(t)
miner, mux, cleanup := createMiner(t)
defer cleanup(false)
waitForMiningState(t, miner, false)
miner.Start(common.HexToAddress("0x12345"))
waitForMiningState(t, miner, true)
Expand All @@ -174,16 +177,19 @@ func TestStartWhileDownload(t *testing.T) {
}

func TestStartStopMiner(t *testing.T) {
miner, _ := createMiner(t)
miner, _, cleanup := createMiner(t)
defer cleanup(false)
waitForMiningState(t, miner, false)
miner.Start(common.HexToAddress("0x12345"))
waitForMiningState(t, miner, true)
miner.Stop()
waitForMiningState(t, miner, false)

}

func TestCloseMiner(t *testing.T) {
miner, _ := createMiner(t)
miner, _, cleanup := createMiner(t)
defer cleanup(true)
waitForMiningState(t, miner, false)
miner.Start(common.HexToAddress("0x12345"))
waitForMiningState(t, miner, true)
Expand All @@ -195,7 +201,8 @@ func TestCloseMiner(t *testing.T) {
// TestMinerSetEtherbase checks that etherbase becomes set even if mining isn't
// possible at the moment
func TestMinerSetEtherbase(t *testing.T) {
miner, mux := createMiner(t)
miner, mux, cleanup := createMiner(t)
defer cleanup(false)
// Start with a 'bad' mining address
miner.Start(common.HexToAddress("0xdead"))
waitForMiningState(t, miner, true)
Expand Down Expand Up @@ -230,7 +237,7 @@ func waitForMiningState(t *testing.T, m *Miner, mining bool) {
t.Fatalf("Mining() == %t, want %t", state, mining)
}

func createMiner(t *testing.T) (*Miner, *event.TypeMux) {
func createMiner(t *testing.T) (*Miner, *event.TypeMux, func(skipMiner bool)) {
// Create Ethash config
config := Config{
Etherbase: common.HexToAddress("123456789"),
Expand Down Expand Up @@ -259,5 +266,14 @@ func createMiner(t *testing.T) (*Miner, *event.TypeMux) {
// Create event Mux
mux := new(event.TypeMux)
// Create Miner
return New(backend, &config, chainConfig, mux, engine, nil, merger), mux
miner := New(backend, &config, chainConfig, mux, engine, nil, merger)
cleanup := func(skipMiner bool) {
bc.Stop()
engine.Close()
pool.Stop()
if !skipMiner {
miner.Close()
}
}
return miner, mux, cleanup
}
2 changes: 1 addition & 1 deletion miner/worker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ func testRegenerateMiningBlock(t *testing.T, chainConfig *params.ChainConfig, en
w, b := newTestWorker(t, chainConfig, engine, rawdb.NewMemoryDatabase(), 0)
defer w.close()

var taskCh = make(chan struct{})
var taskCh = make(chan struct{}, 3)

taskIndex := 0
w.newTaskHook = func(task *task) {
Expand Down
2 changes: 1 addition & 1 deletion node/node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ func TestLifecycleTerminationGuarantee(t *testing.T) {
// on the given prefix
func TestRegisterHandler_Successful(t *testing.T) {
node := createNode(t, 7878, 7979)

defer node.Close()
// create and mount handler
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte("success"))
Expand Down
1 change: 1 addition & 0 deletions rpc/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,7 @@ func TestClientReconnect(t *testing.T) {
// Start a server and corresponding client.
s1, l1 := startServer("127.0.0.1:0")
client, err := DialContext(ctx, "ws://"+l1.Addr().String())
defer client.Close()
if err != nil {
t.Fatal("can't dial", err)
}
Expand Down
3 changes: 3 additions & 0 deletions signer/core/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,9 @@ func TestSignTx(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if len(list) == 0 {
t.Fatal("Unexpected empty list")
}
a := common.NewMixedcaseAddress(list[0])

methodSig := "test(uint)"
Expand Down