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

Conversation

charlesxsh
Copy link
Contributor

This PR fixes following issues:

testRepair goroutine leak

goroutine 73 [select]:
github.com/ethereum/go-ethereum/core.(*BlockChain).update(0xc00025b300)
   /repos/go-ethereum/core/blockchain.go:2443 +0x513
created by github.com/ethereum/go-ethereum/core.NewBlockChain
   /repos/go-ethereum/core/blockchain.go:378 +0xcb9

TestRegisterHandler_Successful goroutine leak

goroutine 49 [select]:
github.com/ethereum/go-ethereum/accounts.(*Manager).update(0xc000198000)
   /repos/go-ethereum/accounts/manager.go:223 +0x15b4
created by github.com/ethereum/go-ethereum/accounts.NewManager
   /repos/go-ethereum/accounts/manager.go:80 +0x585

...omit others

TestStartStopMiner

goroutine 69 [select]:
github.com/ethereum/go-ethereum/core.(*BlockChain).update(0xc000258600)
   /repos/go-ethereum/core/blockchain.go:2443 +0x513
created by github.com/ethereum/go-ethereum/core.NewBlockChain
   /repos/go-ethereum/core/blockchain.go:378 +0xcb9
 
goroutine 70 [select]:
github.com/ethereum/go-ethereum/core.(*TxPool).scheduleReorgLoop(0xc000059800)
   /repos/go-ethereum/core/tx_pool.go:2273 +0x26ec
created by github.com/ethereum/go-ethereum/core.NewTxPool
   /repos/go-ethereum/core/tx_pool.go:304 +0xa0a
 
goroutine 71 [select]:
github.com/ethereum/go-ethereum/core.(*TxPool).loop(0xc000059800)
   /repos/go-ethereum/core/tx_pool.go:936 +0x2065
created by github.com/ethereum/go-ethereum/core.NewTxPool
   /repos/go-ethereum/core/tx_pool.go:322 +0xb05
 
goroutine 72 [select]:
github.com/ethereum/go-ethereum/miner.(*worker).mainLoop(0xc000248000)
   /repos/go-ethereum/miner/worker.go:1792 +0x517c
created by github.com/ethereum/go-ethereum/miner.newWorker
   /repos/go-ethereum/miner/worker.go:232 +0x88e
 
goroutine 73 [select]:
github.com/ethereum/go-ethereum/miner.(*worker).newWorkLoop(0xc000248000, 0x3b9aca00)
   /repos/go-ethereum/miner/worker.go:877 +0x2405
created by github.com/ethereum/go-ethereum/miner.newWorker
   /repos/go-ethereum/miner/worker.go:233 +0x8ba
 
goroutine 74 [select]:
github.com/ethereum/go-ethereum/miner.(*worker).resultLoop(0xc000248000)
   /repos/go-ethereum/miner/worker.go:2267 +0x2c7b
created by github.com/ethereum/go-ethereum/miner.newWorker
   /repos/go-ethereum/miner/worker.go:234 +0x8dc
 
goroutine 75 [select]:
github.com/ethereum/go-ethereum/miner.(*worker).taskLoop(0xc000248000)
   /repos/go-ethereum/miner/worker.go:2025 +0x1795
created by github.com/ethereum/go-ethereum/miner.newWorker
   /repos/go-ethereum/miner/worker.go:235 +0x8fe
 
goroutine 76 [select]:
github.com/ethereum/go-ethereum/miner.(*Miner).update(0xc00002cea0)
   /repos/go-ethereum/miner/miner.go:350 +0x11b6
created by github.com/ethereum/go-ethereum/miner.New
   /repos/go-ethereum/miner/miner.go:79 +0x2ab

TestClientReconnect

goroutine 24 [select]:
github.com/ethereum/go-ethereum/rpc.(*Client).dispatch(0xc00012cc00, 0x937a78, 0xc00012cb00)
   /repos/go-ethereum/rpc/client.go:1323 +0x1ce5
created by github.com/ethereum/go-ethereum/rpc.initClient
   /repos/go-ethereum/rpc/client.go:379 +0x5a6

TestMinerDownloaderFirstFails
same as 3

TestBuildSchema
Node created is missing close.

TestMinerSetEtherbase
same as 3

8
TestCloseMiner
same as 3

9
testSequentialAnnouncements

goroutine 1154 [select]:
github.com/ethereum/go-ethereum/eth/fetcher.(*BlockFetcher).loop(0xc0001ea7e0)
   /repos/go-ethereum/eth/fetcher/block_fetcher.go:2260 +0x1878c
created by github.com/ethereum/go-ethereum/eth/fetcher.(*BlockFetcher).Start
   /repos/go-ethereum/eth/fetcher/block_fetcher.go:239 +0x67

10
TestEmptyBlockShortCircuit
if completing timeout happened first, sending operation of completing will be blocked

goroutine 92 [chan send]:
github.com/ethereum/go-ethereum/eth/fetcher.TestEmptyBlockShortCircuit.func2(0xc0005a2060, 0x1, 0x1)
   /repos/go-ethereum/eth/fetcher/block_fetcher_test.go:1235 +0xa9
github.com/ethereum/go-ethereum/eth/fetcher.(*BlockFetcher).loop(0xc0005da540)
   /repos/go-ethereum/eth/fetcher/block_fetcher.go:2412 +0x1a364
created by github.com/ethereum/go-ethereum/eth/fetcher.(*BlockFetcher).Start
   /repos/go-ethereum/eth/fetcher/block_fetcher.go:239 +0x67

11
TestNatto

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
   panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x6eb54a]
 
goroutine 18 [running]:
testing.tRunner.func1.2(0x7288e0, 0xae5710)
   /usr/local/go/src/testing/testing.go:1143 +0x335
testing.tRunner.func1(0xc000124a00)
   /usr/local/go/src/testing/testing.go:1146 +0x4c2
panic(0x7288e0, 0xae5710)
   /usr/local/go/src/runtime/panic.go:965 +0x1b9
github.com/ethereum/go-ethereum/internal/jsre.TestNatto(0xc000124a00)
   /repos/go-ethereum/internal/jsre/jsre_test.go:100 +0x22a
testing.tRunner(0xc000124a00, 0x78f4e0)
   /usr/local/go/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
   /usr/local/go/src/testing/testing.go:1238 +0x2b5

12
testRegenerateMiningBlock
if new task timeout happened first, sending operation on taskCh will be blocked

goroutine 71 [chan send]:
github.com/ethereum/go-ethereum/miner.testRegenerateMiningBlock.func1(0xc00011f780)
   /repos/go-ethereum/miner/worker_test.go:546 +0xc9
github.com/ethereum/go-ethereum/miner.(*worker).taskLoop(0xc000320000)
   /repos/go-ethereum/miner/worker.go:1917 +0x6c5
created by github.com/ethereum/go-ethereum/miner.newWorker
   /repos/go-ethereum/miner/worker.go:235 +0x8fe

13
TestSignTx
api.List might possibly return both nil

--- FAIL: TestSignTx (0.49s)
panic: runtime error: index out of range [0] with length 0 [recovered]
   panic: runtime error: index out of range [0] with length 0
 
goroutine 23 [running]:
testing.tRunner.func1.2(0xbd2540, 0xc0013b6000)
   /usr/local/go/src/testing/testing.go:1143 +0x335
testing.tRunner.func1(0xc00041f400)
   /usr/local/go/src/testing/testing.go:1146 +0x4c2
panic(0xbd2540, 0xc0013b6000)
   /usr/local/go/src/runtime/panic.go:965 +0x1b9
github.com/ethereum/go-ethereum/signer/core_test.TestSignTx(0xc00041f400)
   /repos/go-ethereum/signer/core/api_test.go:295 +0x162c
testing.tRunner(0xc00041f400, 0xc74200)
   /usr/local/go/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
   /usr/local/go/src/testing/testing.go:1238 +0x2b5

14
TestLightInvalidNumberAnnouncement
if announce timeout happened first, sending operation might be blocked

goroutine 23 [chan send]:
github.com/ethereum/go-ethereum/eth/fetcher.testInvalidNumberAnnouncement.func2(0x37e1ea9b1f2a122b, 0x62fa0e348adc0f38, 0x62e2422bef359fc3, 0xef08972a2a67d48a, 0x1)
   /repos/go-ethereum/eth/fetcher/block_fetcher_test.go:1139 +0x8e
github.com/ethereum/go-ethereum/eth/fetcher.(*BlockFetcher).loop(0xc000146380)
   /repos/go-ethereum/eth/fetcher/block_fetcher.go:2334 +0x1928c
created by github.com/ethereum/go-ethereum/eth/fetcher.(*BlockFetcher).Start
   /repos/go-ethereum/eth/fetcher/block_fetcher.go:239 +0x67

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@charlesxsh
Copy link
Contributor Author

Looks good to me, thanks!

No problem!

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)

Comment on lines 803 to 808
num := number - offset
recent := bc.GetBlockByNumber(num)
if recent == nil {
log.Error("Failed to get block", 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.

@@ -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

@holiman holiman changed the title fix blocking and non-blocking issues core, miner, rpc, eth: fix goroutine leaks in tests Jan 18, 2022
@holiman holiman added this to the 1.10.16 milestone Jan 21, 2022
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman merged commit eef7a33 into ethereum:master Jan 21, 2022
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Jan 21, 2022
* fix blocking and non-blocking issues

* core: revert change in blockchain.go

Co-authored-by: Martin Holst Swende <martin@swende.se>
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
* fix blocking and non-blocking issues

* core: revert change in blockchain.go

Co-authored-by: Martin Holst Swende <martin@swende.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants