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.

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

Refactor defer-close with error handling #378

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

facuMH
Copy link
Contributor

@facuMH facuMH commented Dec 16, 2024

This PR contributes to https://github.com/Fantom-foundation/sonic-admin/issues/90

This PR adds real life use cases of both functions provided the caution package to properly handler errors in case of defer and close.

This PR fixes some of the unhandled errors reported by the external audit. Most (if not all) of them are from either defer close patterns or similar cleanups or unhandled close errors.

Test list to verify we don't have double closes (or other errors) and panic now because error is no longer ignored:

  • run make test
  • Norma test
    • demonet/static.yml
    • demonet/dynamic.yml
  • testing suggestions ?

@facuMH facuMH self-assigned this Dec 16, 2024
@facuMH facuMH requested a review from LuisPH3 December 16, 2024 15:14
cmd/sonictool/app/heal.go Outdated Show resolved Hide resolved
Copy link
Contributor

@LuisPH3 LuisPH3 left a comment

Choose a reason for hiding this comment

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

I saw some details that need handing;

Additionally, I would like to avoid using caution.CloseAndReportError for scenarios without defer, it does nonstandard error handling, and it may be a little convoluted to read. We can talk about a replacement.

cmd/sonictool/app/genesis.go Show resolved Hide resolved
cmd/sonictool/app/sign_genesis.go Outdated Show resolved Hide resolved
@facuMH facuMH requested a review from LuisPH3 December 18, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants