common.Service: review Start's return signature (bool, error)

[ebuchman]

https://github.com/tendermint/go-common/issues/1

[ebuchman]

Why bother returning a bool? Is the error not enough?

https://github.com/tendermint/go-common/blob/master/service.go#L78
@melekes
Member
melekes commented on Mar 21
yeah, bool is superfluous
@ethanfrey
Member
ethanfrey commented on Mar 21
If I remember correctly when I was writing test code, if I call Start() on a Service that is already running, it returns (false, nil). Only if I try to legitimately start it, but it fails in startup do I get an error.

The distinction is quite important to make it safe for reentrant calls. The other approach would be to have a special error type like ErrAlreadyStarted, then check for that in your code explicitly. Kind of like if I make a db call in gorm, and get an error, I check if it is a RecordNotFound error, or whether there was a real error with the db query.
@melekes
Member
melekes commented on Mar 21
Ah, I see. Thanks. I must say I like ErrAlreadyStarted approach more (not just in Golang)

[odeke-em]

I've retitled the issue a little.

IMHO in deed, like @melekes says, returning bool is unnecessary. I'd support the approach of just returning a concrete error like @ethanfrey suggested with ErrAlreadyStarted thereby replacing (bool, error) with just error

Usage

switch err := svc.Start(); err != nil {
  case common.ErrAlreadyStarted:
  case common.ErrAlreadyStopped:
  default:
    // Miscellaneous error handle it
    return nil, err
  case nil:
    // Home free
}

Also I think we can make the code clearer with a