Skip to content

Conversation

@ellemouton
Copy link
Member

Ensure that if any store init in the stores struct creation fails, then any successful stores are still closed during shutdown.

This addresses this review comment

@ellemouton ellemouton self-assigned this Apr 3, 2025
@ellemouton ellemouton added the no-changelog This PR is does not require a release notes entry label Apr 3, 2025
Ensure that if any store init in the `stores` struct creation fails,
then any successful stores are still closed during shutdown.
returnErr = err
if g.stores.firewall != nil {
if err := g.stores.firewall.Stop(); err != nil {
log.Errorf("Error stoppint firewall DB: %v",
Copy link
Contributor

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
Contributor

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
Member Author

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
Member Author

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
Member Author

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
Member Author

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
Contributor

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!

@ellemouton ellemouton requested a review from bitromortac April 4, 2025 07:26
Copy link
Contributor

@ViktorT-11 ViktorT-11 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for fixing this 🙏!

@guggero guggero merged commit 846dc68 into lightninglabs:master Apr 7, 2025
20 of 21 checks passed
@ellemouton ellemouton deleted the sql26 branch April 7, 2025 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR is does not require a release notes entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants