[common] revert started flag when service already stopped

[melekes]

also, return ErrNotStarted when trying to stop a not-running service

[ValarDragon]

I'm confused as to why we have separate started and stopped flags. Shouldn't started = !stopped? It seems like doing that would simplify alot of this code, is that approach not safe with concurrency?

[melekes]

There was a discussion about simplifying Service and @jaekwon made a PR tendermint/tmlibs#114. Not sure why we did not merge it.

[ebuchman]

What motivated this ?

[melekes]

#2258

func(c *rpcClient.WSClient) error {
		if c.IsRunning() {
			c.Stop() // panics on close c.ResponsesCh when not started (=nil)
		}

We should not panic if somebody's trying to Stop a not-started WSClient.

[ValarDragon]

This PR seems good to me, but I'd prefer if we removedstarted and stopped and just had running.

[melekes]

How about we merge this and bring tendermint/tmlibs#114 in?

[ValarDragon]

Lgtm, lets go with your suggestion and bring in the relevant tmlibs PR

[jaekwon]

I think this is fine for now.
The reason why it's still split is to consider an API for restarting services... restarting may require different semantics than starting. Currently these services are run-once-only, so you can't restart a stopped process, and that may be what we want ultimately, and even if not, it's better to have a restricted API and to expand it further. Given the way we use services currently (that there is no restart-ability), we may end up masking bugs by allowing them to be restarted after being stopped.

If we want to allow for services to restart, it may be better to require an explicit "restarting". If in the future we know that (a) we want the ability for services to be started and stopped and started again and (b) we don't want to require explicit "restart" calls, then it would make sense to merge start/stop, but I prefer the way things are for the reasons above.