opencbdc-tx icon indicating copy to clipboard operation
opencbdc-tx copied to clipboard

logger::fatal(...) terminates the program - investigate alternatives

Open AlexRamRam opened this issue 1 year ago • 2 comments

Affected Branch

trunk

Basic Diagnostics

  • [X] I've pulled the latest changes on the affected branch and the issue is still present.

  • [X] The issue is reproducible in docker

Description

It's surprising to a reader that the logger object has the power to terminate the program when fatal() is called.

Consider these alternatives to improve code clarity:

  • remove the exit() call from logger::fatal() and client to invoke exit(1)
  • take a logger::fatal) takes additional exit code parameter
  • rename logger::fatal() to panic() or fatal_system_exit()

Code of Conduct

  • [X] I agree to follow this project's Code of Conduct

AlexRamRam avatar Jun 05 '23 18:06 AlexRamRam

Use of the "fatal" interface for a logging mechanism (to log then system exist) may be a common convention. Golang is one (significant) example:

  • https://pkg.go.dev/log#Fatal

From a personal experience, I do think something like fatal_system_exit() would provide better clarity. However, there is a strong argument to close this issue item if fatal is a well-known convention.

@HalosGhost I will leave the decision of closing this item up to you.

AlexRamRam avatar Oct 11 '23 17:10 AlexRamRam

It's not an unheard-of idiom. But, I've never found it well-structured from a separation-of-concerns perspective (seems odd to me that the logger should really be able to control whether execution continues). I'm not particularly certain there's a better plan without rearchitecting a lot of code (this is one of those small implementation detail decisions that has far-reaching effects if you change it because it violates basic separation-of-concerns).

So, I'm comfortable with this being closed, being fixed by a rename, or proposals for something better.

HalosGhost avatar Oct 16 '23 14:10 HalosGhost