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

fix(server/v2/cometbft): wire app closer #22240

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+36 −13
Open

fix(server/v2/cometbft): wire app closer #22240

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c2af67
fix(server/v2/cometbft): close app
julienrbrt Oct 11, 2024
2afd304
close main db from store/v2
julienrbrt Oct 11, 2024
029d19f
typo
julienrbrt Oct 11, 2024
6e737e6
updates
julienrbrt Oct 11, 2024
9b6e888
updates
julienrbrt Oct 12, 2024
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
3 changes: 3 additions & 0 deletions server/v2/cometbft/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type Consensus[T transaction.Tx] struct {
logger log.Logger
appName, version string
app *appmanager.AppManager[T]
appCloser func() error
txCodec transaction.Codec[T]
store types.Store
streaming streaming.Manager
Expand Down Expand Up @@ -77,6 +78,7 @@ func NewConsensus[T transaction.Tx](
logger log.Logger,
appName string,
app *appmanager.AppManager[T],
appCloser func() error,
mp mempool.Mempool[T],
indexedEvents map[string]struct{},
queryHandlersMap map[string]appmodulev2.Handler,
Expand All @@ -89,6 +91,7 @@ func NewConsensus[T transaction.Tx](
appName: appName,
version: getCometBFTServerVersion(),
app: app,
appCloser: appCloser,
cfg: cfg,
store: store,
logger: logger,
Expand Down
6 changes: 3 additions & 3 deletions server/v2/cometbft/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"cosmossdk.io/core/store"
"cosmossdk.io/core/transaction"
"cosmossdk.io/log"
am "cosmossdk.io/server/v2/appmanager"
"cosmossdk.io/server/v2/appmanager"
"cosmossdk.io/server/v2/cometbft/handlers"
cometmock "cosmossdk.io/server/v2/cometbft/internal/mock"
"cosmossdk.io/server/v2/cometbft/mempool"
Expand Down Expand Up @@ -672,7 +672,7 @@ func setUpConsensus(t *testing.T, gasLimit uint64, mempool mempool.Mempool[mock.
sc := cometmock.NewMockCommiter(log.NewNopLogger(), string(actorName), "stf")
mockStore := cometmock.NewMockStore(ss, sc)

b := am.Builder[mock.Tx]{
b := appmanager.Builder[mock.Tx]{
STF: s,
DB: mockStore,
ValidateTxGasLimit: gasLimit,
Expand All @@ -688,7 +688,7 @@ func setUpConsensus(t *testing.T, gasLimit uint64, mempool mempool.Mempool[mock.
am, err := b.Build()
require.NoError(t, err)

return NewConsensus[mock.Tx](log.NewNopLogger(), "testing-app", am, mempool, map[string]struct{}{}, nil, mockStore, Config{AppTomlConfig: DefaultAppTomlConfig()}, mock.TxCodec{}, "test")
return NewConsensus[mock.Tx](log.NewNopLogger(), "testing-app", am, func() error { return nil }, mempool, map[string]struct{}{}, nil, mockStore, Config{AppTomlConfig: DefaultAppTomlConfig()}, mock.TxCodec{}, "test")
}

// Check target version same with store's latest version
Expand Down
1 change: 1 addition & 0 deletions server/v2/cometbft/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ func (s *CometBFTServer[T]) Init(appI serverv2.AppI[T], cfg map[string]any, logg
s.logger,
appI.Name(),
appI.GetAppManager(),
appI.Close,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistency Detected in NewConsensus Function Signature

The NewConsensus function is being called with an additional appI.Close parameter, but its signature does not reflect this change.

  • File: server/v2/cometbft/server.go
  • Issue: NewConsensus function signature missing the appI.Close parameter.
🔗 Analysis chain

LGTM. Verify consistency of appI.Close usage.

The addition of appI.Close to the NewConsensus function call enhances the application's lifecycle management. This change allows for graceful shutdown of the application.

Please ensure that this change is consistent with the NewConsensus function signature and its usage across the codebase. Run the following script to verify:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of NewConsensus function and appI.Close method

# Test 1: Check the NewConsensus function signature
echo "Checking NewConsensus function signature:"
ast-grep --lang go --pattern 'func NewConsensus($_) $_'

# Test 2: Check for other occurrences of NewConsensus function calls
echo "Checking other NewConsensus function calls:"
rg --type go 'NewConsensus\(' -A 10 -B 2

# Test 3: Check for the definition and usage of Close method in AppI interface
echo "Checking AppI interface for Close method:"
ast-grep --lang go --pattern 'type AppI[T $_] interface {
  $$$
  Close() $_
  $$$
}'

Length of output: 1613

s.serverOptions.Mempool(cfg),
indexEvents,
appI.GetQueryHandlers(),
Expand Down
2 changes: 1 addition & 1 deletion server/v2/testdata/app.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ minimum-gas-prices = '0stake'
app-db-backend = 'goleveldb'

[store.options]
# SState storage database type. Currently we support: "sqlite", "pebble" and "rocksdb"
# State storage database type. Currently we support: "sqlite", "pebble" and "rocksdb"
ss-type = 'sqlite'
# State commitment database type. Currently we support: "iavl" and "iavl-v2"
sc-type = 'iavl'
Expand Down
1 change: 1 addition & 0 deletions server/v2/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,5 @@ type AppI[T transaction.Tx] interface {
GetQueryHandlers() map[string]appmodulev2.Handler
GetStore() store.RootStore
GetSchemaDecoderResolver() decoding.DecoderResolver
Close() error
}
10 changes: 10 additions & 0 deletions simapp/v2/app_di.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,16 @@ func (app *SimApp[T]) TxConfig() client.TxConfig {
return app.txConfig
}

// GetStore returns the root store.
func (app *SimApp[T]) GetStore() store.RootStore {
return app.store
}

// Close overwrites the base Close method to close the stores.
func (app *SimApp[T]) Close() error {
if err := app.store.Close(); err != nil {
return err
}

return app.App.Close()
}
1 change: 1 addition & 0 deletions store/v2/commitment/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const (
)

// MetadataStore is a store for metadata related to the commitment store.
// It isn't metadata store role to close the underlying KVStore.
type MetadataStore struct {
kv corestore.KVStoreWithBatch
}
Expand Down
4 changes: 2 additions & 2 deletions store/v2/root/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const (

// Options are the options for creating a root store.
type Options struct {
SSType SSType `mapstructure:"ss-type" toml:"ss-type" comment:"SState storage database type. Currently we support: \"sqlite\", \"pebble\" and \"rocksdb\""`
SSType SSType `mapstructure:"ss-type" toml:"ss-type" comment:"State storage database type. Currently we support: \"sqlite\", \"pebble\" and \"rocksdb\""`
SCType SCType `mapstructure:"sc-type" toml:"sc-type" comment:"State commitment database type. Currently we support: \"iavl\" and \"iavl-v2\""`
SSPruningOption *store.PruningOption `mapstructure:"ss-pruning-option" toml:"ss-pruning-option" comment:"Pruning options for state storage"`
SCPruningOption *store.PruningOption `mapstructure:"sc-pruning-option" toml:"sc-pruning-option" comment:"Pruning options for state commitment"`
Expand Down Expand Up @@ -177,5 +177,5 @@ func CreateRootStore(opts *FactoryOptions) (store.RootStore, error) {
}

pm := pruning.NewManager(sc, ss, storeOpts.SCPruningOption, storeOpts.SSPruningOption)
return New(opts.Logger, ss, sc, pm, nil, nil)
return New(opts.SCRawDB, opts.Logger, ss, sc, pm, nil, nil)
}
2 changes: 1 addition & 1 deletion store/v2/root/migrate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func (s *MigrateStoreTestSuite) SetupTest() {
pm := pruning.NewManager(sc, ss, nil, nil)

// assume no storage store, simulate the migration process
s.rootStore, err = New(testLog, ss, orgSC, pm, migrationManager, nil)
s.rootStore, err = New(dbm.NewMemDB(), testLog, ss, orgSC, pm, migrationManager, nil)
s.Require().NoError(err)
}

Expand Down
6 changes: 6 additions & 0 deletions store/v2/root/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ type Store struct {
logger corelog.Logger
initialVersion uint64

// holds the db instance for closing it
db corestore.KVStoreWithBatch

// stateStorage reflects the state storage backend
stateStorage store.VersionedDatabase

Expand Down Expand Up @@ -68,6 +71,7 @@ type Store struct {
//
// NOTE: The migration manager is optional and can be nil if no migration is required.
func New(
db corestore.KVStoreWithBatch,
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
logger corelog.Logger,
ss store.VersionedDatabase,
sc store.Committer,
Expand All @@ -76,6 +80,7 @@ func New(
m metrics.StoreMetrics,
) (store.RootStore, error) {
return &Store{
db: db,
logger: logger,
initialVersion: 1,
stateStorage: ss,
Expand All @@ -92,6 +97,7 @@ func New(
func (s *Store) Close() (err error) {
err = errors.Join(err, s.stateStorage.Close())
err = errors.Join(err, s.stateCommitment.Close())
err = errors.Join(err, s.db.Close())

s.stateStorage = nil
s.stateCommitment = nil
Expand Down
6 changes: 3 additions & 3 deletions store/v2/root/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (s *RootStoreTestSuite) SetupTest() {
s.Require().NoError(err)

pm := pruning.NewManager(sc, ss, nil, nil)
rs, err := New(noopLog, ss, sc, pm, nil, nil)
rs, err := New(dbm.NewMemDB(), noopLog, ss, sc, pm, nil, nil)
s.Require().NoError(err)

s.rootStore = rs
Expand All @@ -84,7 +84,7 @@ func (s *RootStoreTestSuite) newStoreWithPruneConfig(config *store.PruningOption

pm := pruning.NewManager(sc, ss, config, config)

rs, err := New(noopLog, ss, sc, pm, nil, nil)
rs, err := New(dbm.NewMemDB(), noopLog, ss, sc, pm, nil, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consistent change, but concerns remain about logger replacement.

This change is consistent with the modification in SetupTest, replacing noopLog with dbm.NewMemDB(). However, the same concerns apply here. Replacing a logger with an in-memory database in the New function call could potentially alter the behavior of pruning tests. Please confirm if this change is intentional and explain how it affects the test setup for pruning configurations.

s.Require().NoError(err)

s.rootStore = rs
Expand All @@ -93,7 +93,7 @@ func (s *RootStoreTestSuite) newStoreWithPruneConfig(config *store.PruningOption
func (s *RootStoreTestSuite) newStoreWithBackendMount(ss store.VersionedDatabase, sc store.Committer, pm *pruning.Manager) {
noopLog := coretesting.NewNopLogger()

rs, err := New(noopLog, ss, sc, pm, nil, nil)
rs, err := New(dbm.NewMemDB(), noopLog, ss, sc, pm, nil, nil)
s.Require().NoError(err)

s.rootStore = rs
Expand Down
4 changes: 2 additions & 2 deletions store/v2/root/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (s *UpgradeStoreTestSuite) SetupTest() {
sc, err := commitment.NewCommitStore(multiTrees, nil, s.commitDB, testLog)
s.Require().NoError(err)
pm := pruning.NewManager(sc, ss, nil, nil)
s.rootStore, err = New(testLog, ss, sc, pm, nil, nil)
s.rootStore, err = New(s.commitDB, testLog, ss, sc, pm, nil, nil)
s.Require().NoError(err)

// commit changeset
Expand Down Expand Up @@ -92,7 +92,7 @@ func (s *UpgradeStoreTestSuite) loadWithUpgrades(upgrades *corestore.StoreUpgrad
sc, err := commitment.NewCommitStore(multiTrees, oldTrees, s.commitDB, testLog)
s.Require().NoError(err)
pm := pruning.NewManager(sc, s.rootStore.GetStateStorage().(store.Pruner), nil, nil)
s.rootStore, err = New(testLog, s.rootStore.GetStateStorage(), sc, pm, nil, nil)
s.rootStore, err = New(s.commitDB, testLog, s.rootStore.GetStateStorage(), sc, pm, nil, nil)
s.Require().NoError(err)
}

Expand Down
2 changes: 1 addition & 1 deletion tools/confix/data/v2-app.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ minimum-gas-prices = '0stake'
app-db-backend = 'goleveldb'

[store.options]
# SState storage database type. Currently we support: "sqlite", "pebble" and "rocksdb"
# State storage database type. Currently we support: "sqlite", "pebble" and "rocksdb"
ss-type = 'sqlite'
# State commitment database type. Currently we support: "iavl" and "iavl-v2"
sc-type = 'iavl'
Expand Down
Loading