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.

Sign up for GitHub

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

Jump to bottom

ethclient/simulated: add test case to ensure that a simulated backend does not leak go-routines after being closed #31033

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

ethclient/simulated: add test case to ensure that a simulated backend does not leak go-routines after being closed #31033

wants to merge 7 commits into from
+22 −0

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Jan 15, 2025 •
edited
Loading

Adds a basic sanity test case to catch any go-routines leaked from instantiation/closing of a simulated backend (as reported in #31022)

@jwasinger jwasinger requested a review from fjl as a code owner January 15, 2025 08:31
@jwasinger jwasinger force-pushed the simulated-backend-gr-leak-check branch from f802e0e to fbd661b Compare January 15, 2025 09:20
@jwasinger jwasinger changed the title ethclient/simulated: add test case to ensure that instantiation of a simulated backend does not leak go-routines. ethclient/simulated: add test case to ensure that a simulated backend does not leak go-routines after being closed Jan 15, 2025
@jwasinger
Copy link
Contributor Author

Currently these are the leaked go-routines that do not have PRs open to fix them:

--- FAIL: TestCheckSimBackendGoroutineLeak (0.02s)
    backend_test.go:395: leaked goroutines:
        github.com/syndtr/goleveldb/leveldb.
        	/Users/jwasinger/go/pkg/mod/github.com/syndtr/[email protected]/leveldb/db_state.go:110 +0xe4
        created by github.com/syndtr/goleveldb/leveldb.openDB in goroutine xxx
        	/Users/jwasinger/go/pkg/mod/github.com/syndtr/[email protected]/leveldb/db.go:149 +0x3d0
        
        github.com/ethereum/go-ethereum/eth/filters.
        	/Users/jwasinger/projects/go-ethereum-master/eth/filters/api.go:93 +0x74
        created by github.com/ethereum/go-ethereum/eth/filters.NewFilterAPI in goroutine xxx
        	/Users/jwasinger/projects/go-ethereum-master/eth/filters/api.go:81 +0xe0

@jwasinger jwasinger force-pushed the simulated-backend-gr-leak-check branch 2 times, most recently from cd31eac to c50e6ba Compare January 20, 2025 10:46
@jwasinger jwasinger mentioned this pull request Jan 21, 2025
Merged
@jwasinger jwasinger force-pushed the simulated-backend-gr-leak-check branch from 53787e3 to 79f067d Compare January 21, 2025 08:35
fjl pushed a commit that referenced this pull request Jan 21, 2025
@jwasinger
eth/filters: ensure API timeoutLoop terminates with event system (#31056
6c10996 
)

Discovered from failing test introduced
#31033 . We should ensure
`timeoutLoop` terminates if the filter event system is terminated.
@jwasinger jwasinger force-pushed the simulated-backend-gr-leak-check branch from 79f067d to c7bdcca Compare January 21, 2025 12:32
@s1na
Copy link
Contributor

s1na commented Jan 21, 2025

Out of curiosity is there reason why you went for a custom solution rather than goleak which the OP used?

@jwasinger
Copy link
Contributor Author

@s1na I was under the impression that the library wasn't sufficient for the case here, but looking into the documentation further: it definitely is and we should use it to simplify this PR.

sebastianst pushed a commit to ethereum-optimism/op-geth that referenced this pull request Jan 23, 2025
@jwasinger @sebastianst
eth/filters: ensure API timeoutLoop terminates with event system (#31…
cff02d5 
…056)

Discovered from failing test introduced
ethereum/go-ethereum#31033 . We should ensure
`timeoutLoop` terminates if the filter event system is terminated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjl fjl Awaiting requested review from fjl fjl is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jwasinger @s1na