fix(state): TxIndexer no longer leaks its database's goroutines.#4642
Merged
fix(state): TxIndexer no longer leaks its database's goroutines.#4642
TxIndexer no longer leaks its database's goroutines.#4642Conversation
added 3 commits
December 10, 2024 13:18
This method is necessary to close the underlying database that a TxIndexer uses. If not called, the TxIndexer will leak the goroutines associated with the database.
They now implement the `Close` method to close their database.
TxIndexer no longer leaks its database's goroutines.TxIndexer no longer leaks its database's goroutines.
alesforz
commented
Dec 10, 2024
5 tasks
jmalicevic
reviewed
Dec 10, 2024
melekes
approved these changes
Dec 11, 2024
jmalicevic
approved these changes
Dec 11, 2024
alesforz
pushed a commit
that referenced
this pull request
Dec 11, 2024
The test unfolds too quickly for the node's indexer to finish its operations. Therefore, the goroutine below that calls n.Stop() will close the indexer's database right when the indexer is trying to write to it at line state/txindex/indexer_service.go:109 thus causing a panic. This small sleep allows the indexer to finish its write operation before the node is shut down. Unfortunately, there is no graceful shutdown available for the database layer, i.e., make the database wait for all the active connections before closing. The test does not fail in `main` because the code in `main` does not close the indexer's database when shutting down a node (which causes its goroutines to leak. See #4642).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
The
TxIndexerinterface defines how to index and search transactions. Its implementers need to interact with a database, which callers are typically expected to close when done. However,TxIndexerdoes not provide aClosemethod. This prevents closing the database used by the transaction indexer, causing goroutines associated with that database to leak.Two tests,
TestInspectConstructorandTestInspectRun, show this problem indirectly. These tests check whether anInspectorleaks goroutines. TheInspectorsets up three databases: a block database, a state database, and a transaction indexer database. It closes only the block and state databases because it cannot close the transaction indexer’s database (because of the missingClosemethod). Therefore, the transaction indexer database's goroutines leak, and the two tests above detect that.In
main, both tests pass because they use aMemDB, an in-memory store that does not spawn goroutines. However, in the work to remove cometbft-db, the tests switch to a “real” database that does spawn goroutines. Without a properClosemethod inTxIndexer, these goroutines remain active, causingTestInspectConstructorandTestInspectRunto fail.This Change
This PR adds a
Closemethod to theTxIndexerinterface and updates the existing implementation to properly close the underlying database. It also modifies the code to callTxIndexer.Closewhere needed, ensuring that database resources are released and preventing goroutine leaks.PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments