gui: Rework create and run lifecycles.

[davecgh]

This requires #370.

This consists of a series of commits that ultimately make the gui webserver respect the context and perform an orderly shutdown when it is canceled as well

• First, it consolidates the logic that creates the GUI instance.

  It's generally good practice to first create everything that can fail and then create the final instance at once with the results versus doing it piecemeal.

  Piecemeal creation is typically more error prone and, while not a huge concern here, it also ends up needlessly creating objects that are just thrown away in the event of a later error.

• Second, it initializes the cache when GUI is created.

  It accomplishes this by updating the init func to accept the config instead of individual components and moving the logic that acquires the details via the config from the Run method into it.

  It also unexports the InitCache method since it is only ever used internal to the gui package.

  Not only does it further consolidate the creation logic in one place, it also exposes any errors back to the main thread when the pool is first initializing so any issues can be corrected immediately.

• Third, it splits out the code that periodically updates the cache and notifies websocket clients into a separate method that is invoked by Run and introduces a waitgroup for it.

• Fourth, itm oves the code that starts the webserver listeners into a separate method that is invoked by Run and removes the server field from the GUI type since it is only needed locally in the new method.

• Finally, it makes the webserver respect the context so it is shutdown in an orderly fashion when the context is canceled and updates the gui Run method to add the goroutine to the waitgroup now that it will be shutdown as expected.

[jholdstock]

Haven't given this a full review yet, but just want to give you a heads up that I still don't think this will be the final form of the web server lifecycle. There is an existing issue which I think should be addressed.

In the case where the GUI is configured to listen on a port which is already in use (a reasonably common scenario) runWebServer will return immediately. The rest of the dcrpool process will continue to run. IMO the process should exit(1) in this scenario so the admin can investigate and fix it.

[jholdstock]

In this vspd PR I am checking that the port is available by starting the TCP listener in .New, then starting the server itself in .Run. Any thoughts on that approach?

[davecgh]

Haven't given this a full review yet, but just want to give you a heads up that I still don't think this will be the final form of the web server lifecycle. There is an existing issue which I think should be addressed.

In the case where the GUI is configured to listen on a port which is already in use (a reasonably common scenario) runWebServer will return immediately. The rest of the dcrpool process will continue to run. IMO the process should exit(1) in this scenario so the admin can investigate and fix it.

Right. I intentionally maintained the existing behavior for that since I was trying to avoid changing too many semantics at once.

That said, I do agree with you. Although the pool can technically work without the GUI, it wouldn't realistically be very useful without it. I very much prefer the fail early approach so that sysadmins can resolve any issues immediately versus having to notice an error in a log message and then wondering why the GUI doesn't seem to be working.

[davecgh]

In this vspd PR I am checking that the port is available by starting the TCP listener in .New, then starting the server itself in .Run. Any thoughts on that approach?

Yes, that is almost precisely what I do in dcrd:

func newServer(ctx context.Context, listenAddrs []string, db database.DB, ....) (*server, error) {
...
    var listeners []net.Listener
    var nat *upnpNAT
    if !cfg.DisableListen {
        var err error
        listeners, nat, err = initListeners(ctx, chainParams, amgr, listenAddrs,
            services)
        if err != nil {
            return nil, err
        }
        if len(listeners) == 0 {
            return nil, errors.New("no valid listen address")
        }
    }

Obviously none of the nat bits apply here, but just showing that it's the model dcrd uses.

EDIT:

What I would probably do nowadays though is initialize the listeners outside of newServer and pass them in so the context param could be avoided on newX so there is no temptation to shove it into a struct or create subcontexts from it, etc.

In the end, it's not all that much different as long as it's implemented correctly, but I've found that doing all of the setup of long-running things that are required like listeners, required connections to other services, etc, to be much easier to reason about when it's done right in the main func as opposed to encapsulating it all in constructors that live all over the place.

I imagine that is a matter of taste, because I know many prefer to see a "clean" main func that delegates everything.

It's similar, imo, to the way people have differing opinions on exceptions versus error returns. I personally hate exceptions. They obfuscate control flow by creating an invisible second control flow through your programs which ultimately makes them much harder to reason about. It is much easier to create robust software using error returns because you know exactly what does and doesn't return an error and whether or not they're handled right there at the call site.

[jholdstock]

All my questions above are answered, and the code is looking good apart from one last thing.server.Shutdown doco says:

Make sure the program doesn't exit and waits instead for Shutdown to return.

An easy way to do that is to swap the code which is blocking and the code in the goroutine. So that would something look like...

go server.Listen()
<-ctx.Done()
server.Shutdown()
return

[davecgh]

All my questions above are answered, and the code is looking good apart from one last thing.server.Shutdown doco says:

Make sure the program doesn't exit and waits instead for Shutdown to return.

Updated. I still think all of the listener logic should be in the constructor or even in the main method and passed in, but that can be another PR.