Skip to content

lit: make sure to always close stores on errored start-up #1026

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

Merged
guggero merged 1 commit into from
Apr 7, 2025
+75 −80
Merged

lit: make sure to always close stores on errored start-up #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 33 additions & 41 deletions config_dev.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ func defaultDevConfig() *DevConfig {
func NewStores(cfg *Config, clock clock.Clock) (*stores, error) {
var (
networkDir = filepath.Join(cfg.LitDir, cfg.Network)
acctStore accounts.Store
sessStore session.Store
closeFns = make(map[string]func() error)
stores = &stores{
closeFns: make(map[string]func() error),
}
)

switch cfg.DatabaseBackend {
Expand All @@ -98,76 +98,68 @@ func NewStores(cfg *Config, clock clock.Clock) (*stores, error) {
// the directory where we will store the database file exists.
err := makeDirectories(networkDir)
if err != nil {
return nil, err
return stores, err
}

sqlStore, err := db.NewSqliteStore(cfg.Sqlite)
if err != nil {
return nil, err
return stores, err
}

acctStore = accounts.NewSQLStore(sqlStore.BaseDB, clock)
sessStore = session.NewSQLStore(sqlStore.BaseDB, clock)
closeFns["sqlite"] = sqlStore.BaseDB.Close
acctStore := accounts.NewSQLStore(sqlStore.BaseDB, clock)
sessStore := session.NewSQLStore(sqlStore.BaseDB, clock)

stores.accounts = acctStore
stores.sessions = sessStore
stores.closeFns["sqlite"] = sqlStore.BaseDB.Close

case DatabaseBackendPostgres:
sqlStore, err := db.NewPostgresStore(cfg.Postgres)
if err != nil {
return nil, err
return stores, err
}

acctStore = accounts.NewSQLStore(sqlStore.BaseDB, clock)
sessStore = session.NewSQLStore(sqlStore.BaseDB, clock)
closeFns["postgres"] = sqlStore.BaseDB.Close
acctStore := accounts.NewSQLStore(sqlStore.BaseDB, clock)
sessStore := session.NewSQLStore(sqlStore.BaseDB, clock)

stores.accounts = acctStore
stores.sessions = sessStore
stores.closeFns["postgres"] = sqlStore.BaseDB.Close

default:
accountStore, err := accounts.NewBoltStore(
filepath.Dir(cfg.MacaroonPath), accounts.DBFilename,
clock,
)
if err != nil {
return nil, err
return stores, err
}
closeFns["bbolt-accounts"] = accountStore.Close

stores.accounts = accountStore
stores.closeFns["bbolt-accounts"] = accountStore.Close

sessionStore, err := session.NewDB(
networkDir, session.DBFilename, clock, accountStore,
)
if err != nil {
return nil, err
return stores, err
}
closeFns["bbolt-sessions"] = sessionStore.Close

acctStore = accountStore
sessStore = sessionStore
stores.sessions = sessionStore
stores.closeFns["bbolt-sessions"] = sessionStore.Close
}

firewallBoltDB, err := firewalldb.NewBoltDB(
networkDir, firewalldb.DBFilename, sessStore,
networkDir, firewalldb.DBFilename, stores.sessions,
)
if err != nil {
return nil, fmt.Errorf("error creating firewall BoltDB: %v",
return stores, fmt.Errorf("error creating firewall BoltDB: %v",
err)
}
closeFns["bbolt-firewalldb"] = firewallBoltDB.Close

return &stores{
accounts: acctStore,
sessions: sessStore,
firewall: firewalldb.NewDB(firewallBoltDB),
firewallBolt: firewallBoltDB,
close: func() error {
var returnErr error
for storeName, fn := range closeFns {
err := fn()
if err != nil {
log.Errorf("error closing %s store: %v",
storeName, err)
returnErr = err
}
}

return returnErr
},
}, nil

stores.firewall = firewalldb.NewDB(firewallBoltDB)
stores.firewallBolt = firewallBoltDB
stores.closeFns["bbolt-firewalldb"] = firewallBoltDB.Close

return stores, nil
}
49 changes: 16 additions & 33 deletions config_prod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,55 +32,38 @@ func (c *DevConfig) Validate(_, _ string) error {
func NewStores(cfg *Config, clock clock.Clock) (*stores, error) {
networkDir := filepath.Join(cfg.LitDir, cfg.Network)

stores := &stores{
closeFns: make(map[string]func() error),
}

acctStore, err := accounts.NewBoltStore(
filepath.Dir(cfg.MacaroonPath), accounts.DBFilename, clock,
)
if err != nil {
return nil, err
return stores, err
}
stores.accounts = acctStore
stores.closeFns["accounts"] = acctStore.Close

sessStore, err := session.NewDB(
networkDir, session.DBFilename, clock, acctStore,
)
if err != nil {
return nil, fmt.Errorf("error creating session BoltStore: %v",
err)
return stores, fmt.Errorf("error creating session BoltStore: "+
"%v", err)
}
stores.sessions = sessStore
stores.closeFns["sessions"] = sessStore.Close

firewallDB, err := firewalldb.NewBoltDB(
networkDir, firewalldb.DBFilename, sessStore,
)
if err != nil {
return nil, fmt.Errorf("error creating firewall DB: %v", err)
return stores, fmt.Errorf("error creating firewall DB: %v", err)
}
stores.firewallBolt = firewallDB
stores.firewall = firewalldb.NewDB(firewallDB)
stores.closeFns["firewall"] = firewallDB.Close

return &stores{
accounts: acctStore,
sessions: sessStore,
firewallBolt: firewallDB,
firewall: firewalldb.NewDB(firewallDB),
close: func() error {
var returnErr error
if err := acctStore.Close(); err != nil {
returnErr = fmt.Errorf("error closing "+
"account store: %v", err)

log.Error(returnErr.Error())
}
if err := sessStore.Close(); err != nil {
returnErr = fmt.Errorf("error closing "+
"session store: %v", err)

log.Error(returnErr.Error())
}
if err := firewallDB.Close(); err != nil {
returnErr = fmt.Errorf("error closing "+
"firewall DB: %v", err)

log.Error(returnErr.Error())
}

return returnErr
},
}, nil
return stores, nil
}
32 changes: 26 additions & 6 deletions terminal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,25 @@ type stores struct {
firewall *firewalldb.DB
firewallBolt *firewalldb.BoltDB

// close is a callback that can be used to close all the stores in the
// stores struct.
close func() error
// closeFns holds various callbacks that can be used to close any open
// stores in the stores struct.
closeFns map[string]func() error
}

// close closes all open stores in the stores struct. It returns an error if
// any of the stores could not be closed.
func (s *stores) close() error {
var returnErr error
for storeName, closeFn := range s.closeFns {
err := closeFn()
if err != nil {
log.Errorf("error closing %s store: %v",
storeName, err)
returnErr = err
}
}

return returnErr
}

// Run starts everything and then blocks until either the application is shut
Expand Down Expand Up @@ -1450,9 +1466,13 @@ func (g *LightningTerminal) shutdownSubServers() error {
}

if g.stores != nil {
if err := g.stores.firewall.Stop(); err != nil {
log.Errorf("Error stoppint firewall DB: %v", err)
returnErr = err
if g.stores.firewall != nil {
if err := g.stores.firewall.Stop(); err != nil {
log.Errorf("Error stoppint firewall DB: %v",
Copy link
Copy Markdown
Contributor

@bitromortac bitromortac Apr 3, 2025

Choose a reason for hiding this comment

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

stoppint -> stopping

Could we combine this with the firewall's close function?

	stores.closeFns["firewall"] = func() error {
		err := stores.firewall.Stop()
		if err != nil {
			return fmt.Errorf("error stopping firewall DB: %v", err)
		}

		return firewallDB.Close()
	}

Copy link
Copy Markdown
Contributor

@bitromortac bitromortac Apr 3, 2025

Choose a reason for hiding this comment

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

or do we actually need closeFns, since we have access to all stores and can call their Close/Stop methods when shutting down?

Copy link
Copy Markdown
Member Author

@ellemouton ellemouton Apr 4, 2025

Choose a reason for hiding this comment

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

Could we combine this with the firewall's close function?

The reason i did not do that is cause we Start it outside. So feels like the same layer should be responsible for calling Stop.

Copy link
Copy Markdown
Member Author

@ellemouton ellemouton Apr 4, 2025

Choose a reason for hiding this comment

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

stoppint -> stopping

Will fix on next push after more review

Copy link
Copy Markdown
Member Author

@ellemouton ellemouton Apr 4, 2025

Choose a reason for hiding this comment

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

or do we actually need closeFns, since we have access to all stores and can call their Close/Stop methods when shutting down?

we could - but then need to do nil check for each. just thought that this was cleaner

Copy link
Copy Markdown
Member Author

@ellemouton ellemouton Apr 4, 2025

Choose a reason for hiding this comment

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

also, remember that this is all temporary - we're just doing it like this while we have these 2 versions. We will later force the migration and only have one. At that point, we dont need the stores struct and can handle each store individually in the terminal struct again

Copy link
Copy Markdown
Contributor

@bitromortac bitromortac Apr 7, 2025

Choose a reason for hiding this comment

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

Thanks for the explanation, looks good!

err)

returnErr = err
}
}

err = g.stores.close()
Expand Down
Loading