WIP Changed Service interface #114
Conversation
common/service.go
Outdated
|
|
||
| The caller must ensure that Start and Stop are not called concurrently. | ||
| Services can be started, then stopped, then optionally restarted. |
There was a problem hiding this comment.
Oops, no it can't be restarted.
|
|
||
| String() string | ||
|
|
||
| SetServiceCore(ServiceCore) |
There was a problem hiding this comment.
I like the new usage of ServiceCore. However, shouldn't we also make ServiceCore implement the String() method if Service implicitly invokes OnStart() and OnStop() of the ServiceCore?
There was a problem hiding this comment.
The String is just the name passed into NewBaseService
| <-bs.Quit | ||
| // SetServiceCore impleents SetServiceCore | ||
| func (bs *BaseService) SetServiceCore(service ServiceCore) { | ||
| bs.impl = service |
There was a problem hiding this comment.
While I see an atomic protection for started and stopped, there is none for logger and impl which sure by contract will only be set once, but if we've got mutual exclusion for some fields, why not for all of them?
This makes sure that our service our bullet proof and not reliant on the user, who can potentially breach the contract.
There was a problem hiding this comment.
We should clarify which things should and shouldn't be called multiple times or concurrently in the comments, but it might be confusing to make things thread safe when they shouldnt be
|
@odeke-em I like the idea of protecting everything. What do you think about just removing SetLogger and SetServiceCore from the interface. Both need to be passed into the constructor and then it can only be started and stopped. |
|
Just noting that we'll want to expose the quit chan somehow. Maybe |
|
|
||
| String() string | ||
|
|
||
| SetServiceCore(ServiceCore) |
There was a problem hiding this comment.
The String is just the name passed into NewBaseService
| <-bs.Quit | ||
| // SetServiceCore impleents SetServiceCore | ||
| func (bs *BaseService) SetServiceCore(service ServiceCore) { | ||
| bs.impl = service |
There was a problem hiding this comment.
We should clarify which things should and shouldn't be called multiple times or concurrently in the comments, but it might be confusing to make things thread safe when they shouldnt be
|
@greg-szabo let's add this to next release (not 0.16) and discuss on monday |
|
damn we forgot to talk about this. maybe we do need to use an organization level kanban @greg-szabo @jolesbi |
|
@greg-szabo can you put it on the tendermint meeting agenda for next week? alternatively, someone can set up a call to discuss. #zoomzoom 🚗 |
|
We should move the Service to Tendermint. It's the only consumer AFAIK. Same with the timers! |
|
this PR is now captured in tendermint/tendermint#1882 but let's leave it open for now so the branch doesn't get deleted, or close and delete if obsolete |
Service could be much simplified.
We shouldn't be masking race conditions by letting the caller call Start/Stop willy nilly. If one needs to rely on such patterns, it's probably because one has racy buggy software.
Now that we know better, lets refactor and test Tendermint to see how racy it is.
We don't use Restart as far as I know.