Startup related fixes #1126
Startup related fixes #1126
Conversation
Pull Request Test Coverage Report for Build 10907521166Details
💛 - Coveralls |
| select { | ||
| case <-s.ready: | ||
| return nil | ||
| case <-s.quit: |
There was a problem hiding this comment.
I think this doesn't solve the tapd: don't set initialized=true when startup (StartAsSubserver) fails part of the issue. We don't close the quit channel when we fail to start there (which is the main entrypoint for litd).
| if info.Server == nil { | ||
| return nil, fmt.Errorf("server is nil, can't check " + | ||
| "RPC state") | ||
|
|
There was a problem hiding this comment.
nit: linter complains about superfluous empty line.
| @@ -413,6 +413,12 @@ func (r *InterceptorChain) rpcStateUnaryServerInterceptor() grpc.UnaryServerInte | |||
| return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, | |||
| handler grpc.UnaryHandler) (interface{}, error) { | |||
|
|
|||
| if info.Server == nil { | |||
| return nil, fmt.Errorf("server is nil, can't check " + | |||
There was a problem hiding this comment.
Can probably be a bit less specific in the error message here, perhaps just "cannot handle call, server not ready"?
| case <-s.quit: | ||
| return fmt.Errorf("tapd is shutting down") | ||
|
|
||
| default: |
There was a problem hiding this comment.
Nice fix. This probably explains one class of potential errors we had.
|
|
||
| default: | ||
| select { | ||
| case <-s.ready: |
There was a problem hiding this comment.
Maybe mention here that we now wait until the server becomes ready or shuts down (meaning this is blocking).
| func (s *Server) CanHandle(msg protofsm.PeerMsg) bool { | ||
| <-s.ready | ||
| func (s *Server) CanHandle(msg protofsm.PeerMsg) (bool, error) { | ||
| err := s.waitForReady() |
There was a problem hiding this comment.
Depending on the discussion on the lnd PR it might be okay to just return false if s.waitForReady() returns an error. But calling s.waitForReady() should definitely be done so we don't block indefinitely.
This PR fixes some startup related bugs that were discovered when integrating tapd with litd.
Read more here
Closes #1122