diff --git a/privval/signer_client.go b/privval/signer_client.go index 5eb043048..bedb52dbc 100644 --- a/privval/signer_client.go +++ b/privval/signer_client.go @@ -29,13 +29,13 @@ func NewSignerClient(endpoint *SignerListenerEndpoint) (*SignerClient, error) { } } - // TODO: Fix this + // TODO(jleni): Fix this //// retrieve and memoize the consensus public key once. //pubKey, err := getPubKey(conn) //if err != nil { // return nil, cmn.ErrorWrap(err, "error while retrieving public key for remote signer") //} - // TODO: Fix this + // TODO(jleni): Fix this //return &SignerClient{endpoint: endpoint, consensusPubKey: pubKey,}, nil return &SignerClient{endpoint: endpoint}, nil diff --git a/privval/signer_client_test.go b/privval/signer_client_test.go index a4d8804f5..738d04108 100644 --- a/privval/signer_client_test.go +++ b/privval/signer_client_test.go @@ -125,7 +125,7 @@ func TestSignerVoteResetDeadline(t *testing.T) { require.NoError(t, tc.signer.SignVote(tc.chainID, have)) assert.Equal(t, want.Signature, have.Signature) - // TODO: Clarify what is actually being tested + // TODO(jleni): Clarify what is actually being tested // This would exceed the deadline if it was not extended by the previous message time.Sleep(testTimeoutReadWrite2o3) @@ -222,7 +222,7 @@ func (ss *BrokenSignerDialerEndpoint) writeMessage(msg RemoteSignerMsg) (err err func TestSignerUnexpectedResponse(t *testing.T) { for _, tc := range getSignerTestCases(t) { func() { - // TODO: This test is actually not working. Fails for a different reason + // TODO(jleni): This test is actually not working. Fails for a different reason tc.signerService.privVal = types.NewErroringMockPV() tc.mockPV = types.NewErroringMockPV() diff --git a/privval/signer_dialer_endpoint.go b/privval/signer_dialer_endpoint.go index 7533b3e44..7d430ddcf 100644 --- a/privval/signer_dialer_endpoint.go +++ b/privval/signer_dialer_endpoint.go @@ -31,7 +31,7 @@ func SignerServiceEndpointConnRetries(retries int) SignerServiceEndpointOption { return func(ss *SignerDialerEndpoint) { ss.maxConnRetries = retries } } -// TODO: Create a common type for a signerEndpoint (common for both listener/dialer) +// TODO(jleni): Create a common type for a signerEndpoint (common for both listener/dialer) // getConnection // AcceptNewConnection // read @@ -102,6 +102,7 @@ func (ss *SignerDialerEndpoint) OnStop() { if ss.conn != nil { if err := ss.conn.Close(); err != nil { ss.Logger.Error("OnStop", "err", cmn.ErrorWrap(err, "closing listener failed")) + ss.Logger.Debug("Reset conn") ss.conn = nil } } @@ -117,6 +118,8 @@ func (ss *SignerDialerEndpoint) serviceLoop() { select { default: { + ss.Logger.Debug("Try connect", "retries", retries, "max", ss.maxConnRetries) + if retries > ss.maxConnRetries { ss.Logger.Error("Maximum retries reached", "retries", retries) return @@ -124,8 +127,9 @@ func (ss *SignerDialerEndpoint) serviceLoop() { if ss.conn == nil { ss.conn, err = ss.dialer() + if err != nil { - ss.Logger.Error("SignerDialerEndpoint::serviceLoop", "err", err) + ss.conn = nil // Explicitly set to nil because dialer returns an interface (https://golang.org/doc/faq#nil_error) retries += 1 continue } @@ -144,7 +148,7 @@ func (ss *SignerDialerEndpoint) serviceLoop() { } func (ss *SignerDialerEndpoint) readMessage() (msg RemoteSignerMsg, err error) { - // TODO: Avoid duplication. Unify endpoints + // TODO(jleni): Avoid duplication. Unify endpoints if ss.conn == nil { return nil, fmt.Errorf("not connected") } @@ -152,6 +156,7 @@ func (ss *SignerDialerEndpoint) readMessage() (msg RemoteSignerMsg, err error) { // Reset read deadline deadline := time.Now().Add(ss.timeoutReadWrite) ss.Logger.Debug("SignerDialerEndpoint: readMessage", "deadline", deadline) + err = ss.conn.SetReadDeadline(deadline) if err != nil { return @@ -167,7 +172,7 @@ func (ss *SignerDialerEndpoint) readMessage() (msg RemoteSignerMsg, err error) { } func (ss *SignerDialerEndpoint) writeMessage(msg RemoteSignerMsg) (err error) { - // TODO: Avoid duplication. Unify endpoints + // TODO(jleni): Avoid duplication. Unify endpoints if ss.conn == nil { return fmt.Errorf("not connected") diff --git a/privval/signer_listener_endpoint.go b/privval/signer_listener_endpoint.go index e19b2deb8..b2cf4bb89 100644 --- a/privval/signer_listener_endpoint.go +++ b/privval/signer_listener_endpoint.go @@ -13,7 +13,6 @@ import ( // SignerValidatorEndpointOption sets an optional parameter on the SocketVal. type SignerValidatorEndpointOption func(*SignerListenerEndpoint) - // SignerListenerEndpoint listens for an external process to dial in // and keeps the connection alive by dropping and reconnecting type SignerListenerEndpoint struct { @@ -28,10 +27,10 @@ type SignerListenerEndpoint struct { // NewSignerListenerEndpoint returns an instance of SignerListenerEndpoint. func NewSignerListenerEndpoint(logger log.Logger, listener net.Listener) *SignerListenerEndpoint { - + sc := &SignerListenerEndpoint{ - listener: listener, - timeoutReadWrite : defaultTimeoutReadWriteSeconds * time.Second, + listener: listener, + timeoutReadWrite: defaultTimeoutReadWriteSeconds * time.Second, } sc.BaseService = *cmn.NewBaseService(logger, "SignerListenerEndpoint", sc) @@ -68,7 +67,7 @@ func (ve *SignerListenerEndpoint) IsConnected() bool { // WaitForConnection waits maxWait for a connection or returns a timeout error func (ve *SignerListenerEndpoint) WaitForConnection(maxWait time.Duration) error { - // TODO: complete this + // TODO(jleni): complete this return nil } @@ -126,7 +125,7 @@ func (ve *SignerListenerEndpoint) SendRequest(request RemoteSignerMsg) (RemoteSi // IsConnected indicates if there is an active connection func (ve *SignerListenerEndpoint) isConnected() bool { -// return ve.IsRunning() && ve.conn != nil + // return ve.IsRunning() && ve.conn != nil return ve.conn != nil } diff --git a/privval/signer_listener_endpoint_test.go b/privval/signer_listener_endpoint_test.go index 98fc0c4ac..405746eb3 100644 --- a/privval/signer_listener_endpoint_test.go +++ b/privval/signer_listener_endpoint_test.go @@ -33,12 +33,13 @@ type dialerTestCase struct { func TestSignerRemoteRetryTCPOnly(t *testing.T) { var ( attemptCh = make(chan int) - retries = 2 + retries = 10 ) ln, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) + // Continuously Accept connection and close {attempts} times go func(ln net.Listener, attemptCh chan<- int) { attempts := 0 for { @@ -68,7 +69,8 @@ func TestSignerRemoteRetryTCPOnly(t *testing.T) { SignerServiceEndpointTimeoutReadWrite(time.Millisecond)(serviceEndpoint) SignerServiceEndpointConnRetries(retries)(serviceEndpoint) - assert.Equal(t, serviceEndpoint.Start(), ErrDialRetryMax) + err = serviceEndpoint.Start() + assert.NoError(t, err) select { case attempts := <-attemptCh: diff --git a/tools/tm-signer-harness/internal/test_harness.go b/tools/tm-signer-harness/internal/test_harness.go index 5c577fa17..a6677e82e 100644 --- a/tools/tm-signer-harness/internal/test_harness.go +++ b/tools/tm-signer-harness/internal/test_harness.go @@ -106,9 +106,9 @@ func NewTestHarness(logger log.Logger, cfg TestHarnessConfig) (*TestHarness, err return nil, newTestHarnessError(ErrFailedToCreateListener, err, "") } - signerClient, err:= privval.NewSignerClient(spv) - if err!=nil { - return nil, newTestHarnessError(ErrFailedToCreateListener, err, "") + signerClient, err := privval.NewSignerClient(spv) + if err != nil { + return nil, newTestHarnessError(ErrFailedToCreateListener, err, "") } return &TestHarness{