Open kyriediculous opened 5 years ago
Generally I agree that the livepeer.go
could use some cleanup ; see https://github.com/livepeer/go-livepeer/issues/710
Idiomaticly the main function should be kept short.
I don't really agree with this reason in itself; while it certainly helps to break out related pieces of logic into their own functions (since that's what functions are for, after all), doing this for the sake of "keeping functions short" usually results in unnecessarily deep call stacks and/or more overly-long helper functions, or code that's so disjoint it impedes clarity.
Extract the startup of various services into helpers. Such as parseFlags
To achieve the "shape" you're talking about implies putting nearly all common state in a giant struct or making it all global, or having functions with lots of parameters and return values.
I'd be OK with putting the parsed flags themselves in a struct (since that's also a way for us to sneak in features such as configuring via file (https://github.com/livepeer/go-livepeer/issues/101), but I'm curious if there is additional common state that we'd end up passing everywhere?
See this discussion in #933
Just curious: for the the pricing flags from that discussion, is there anything prior to that check which really ought to be defer
'd to completion? If so, could the pricing flags be checked a bit earlier, so we can still exit
safely? In the short term, this would be good to keep in mind as a rule when reviewing changes to flags.
os.Exit(1)
It's customary for (Unix) programs to return error codes based on the type of failure; this gives external tools some more information about why a program may have failed. I've thought about using this to help make our CLI tests more specific, but that's a separate issue.
doing this for the sake of "keeping functions short"
To clarify, I'm talking explicitly about the main function, not other functions. In my opinion, the main function should be used to fire up the program and nothing else.
Perhaps I should remove the numbering in the original post, as I did not want to imply that this is the most important reason. The most important reason should be maintainability and being able to do proper error handling (as was the conclusion after the discussion I had with Yondon), the fact that it becomes short is just a positive side effect I guess.
In my opinion Cobra generated CLI programs are a great example of this.
To achieve the "shape" you're talking about implies putting nearly all common state in a giant struct or making it all global, or having functions with lots of parameters and return values.
I was personally thinking of making those global rather than a wrapping struct. Using following syntax:
var (
network *string
rtmpAddr *string
cliAddr *string
)
Altough these don't need to be pointers if we dereference when calling flag.Type()
in a possible parseFlags
helper.
Just curious: for the the pricing flags from that discussion
Yeah on second consideration when skimming through the main function these could be better placed but would need some duplication of e.g. offchain/onchain checks and what type of node is being ran. The thing is that the checks imposed on the pricing flags actually use panic
which implies safe exit
but the returned errors aren't as nice.
In other places in the main we are still using fatal
for e.g. serviceURI checks, etc. where exit
isn't happening safely.
It's customary for (Unix) programs to return error codes based on the type of failure; this gives external tools some more information about why a program may have failed.
Oh yes, that would be nice. I didn't think of that. We could scope that in, considering both issues aren't priorities but just code optimisations down the line.
+1 for refactoring the main
function by adding proper data structures and moving away inline functions. It's 959 lines currently, not easy to work on. There are no good reasons to have any function this long.
Is your feature request related to a problem? Please describe.
We should prevent the main function from terminating before deferred functions can be executed. Currently we are mainly using
glog.Fatal
which is equivalent to callingos.Exit(1)
, this exits the program not allowing any deferred functions to run.In some statements
panic
is used, which executes deferred functions after throwing but it is not idiomatic to panic in such contexts.Idiomatically the main function should be kept short and purely used for startup of the program. Startup of services should be initiated through helpers functions.
We could refactor the cluttered main to simply
glog.Error + return
instead ofglog.Fatal / os.Exit(1)
but this makes testing the returned error codes intest_args.sh
impossibleAdditionally refactoring main makes it easier to maintain the main function and add functionality for the startup of services as needed.
Describe the solution you'd like Extract the startup of various services into helpers. Such as
parseFlags
startDBservice
startEthClient
setPriceInfo
startMediaServer
initS3storage
startLivepeerNode
All error statements should be refactored to match the following pattern
If the helper returns it's deferred functions will execute. We can then safely os.Exit() in the main after encountering an error
eg.
This is the same pattern used by Cobra: eg.
Additional context See this discussion in #933