Skip to content

FR: Convert Fatals in main.go to something that wont loop #261

@thebeline

Description

@thebeline

After developing and testing #256 and switching back to 2.7.0-dev, I encountered an OctoScreen exit/restart loop. When trying to diagnose it, I incorrectly created #260 and #259. Please disregard those. What follows is what really happen, and a note for the future.

Turns out, the real cause for the loop I experienced was simply the calls to logger.Fatalf() inside getSize(). My previous 'issues' and 'PRs' regarding the matter are kind of moot at this point, but I do think we should re-imagine that behaviour.

After realizing my error, and looking back through the code with a clearer head, I noticed the Fatalf behaviour on incorrect Environment Variables was fairly common in main() and I think this is inefficient and obsfuscicates the issue.

Calling Fatalf on an Environment Variable issue (or any of the Fatal error logs) immediately calls exit(), which causes the Service Daemon to restart, which, if the error is not resolved, just calls Fatalf again, filling the log and blinking OctoScreen.

My previous issue and PR on the matter was an attempt to resolve my explicit issue, but I think we should explore a more universal handling of Environment Variable (or, indeed, Init and Main) error handling.

Thoughts

On one hand, perhaps one environment method that loads and validates the available environment variables. This would likely require special methods to validate each one, which would contain the format and validity logic, as well as weather or not it should return the "default".

In that primary environment method, we would have a defer that catches panic, and if it does, we display the Error Window. If it doesn't, it sets the internal variables to either the passed values or the defaults. Maybe this could be an Environment object, and it also contains the variable names, defaults... Hmmm... We call GetInstance which returns the instance, when we call it inside the primary main() we have a defer() that catches and displays the window...

Yeah, something like that.

Additionally, there should probably be a name change on the FatalErrorWindow front, as Fatal implies an exit, and... That's not exactly correct.

Either way, it is all existing behavior, and hasn't really caused TOO much issue thus far, so it is probably fine to leave for now.

The PR that I was testing that lead me to find this actually adds a value for RESOLUTION and handles it, unless someone puts something else invalid in there, they won't hit this issue. And if they do, the behaviour is the same as it has been, so I don't think this discussion is a blocker for that PR.

But it is something to consider for maybe some other time.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugFeatures do not work properly

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions