From fa522ca323c8d4f3aa5d1f26a80bca417b09d79f Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 10 Nov 2020 15:10:12 +0400 Subject: [PATCH] privval: increase read/write timeout to 5s and calculate ping interval based on it (#5638) Partially closes #5550 --- CHANGELOG_PENDING.md | 3 +- privval/signer_dialer_endpoint.go | 20 ++++++------ privval/signer_endpoint.go | 2 +- privval/signer_listener_endpoint.go | 39 ++++++++++++++++++------ privval/signer_listener_endpoint_test.go | 6 +++- privval/socket_listeners.go | 3 +- 6 files changed, 50 insertions(+), 23 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 4746fa3a9..48f296b82 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -25,8 +25,9 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### IMPROVEMENTS - [crypto/ed25519] \#5632 Adopt zip215 `ed25519` verification. (@marbar3778) -- [privval] \#5603 Add `--key` to `init`, `gen_validator`, `testnet` & `unsafe_reset_priv_validator` for use in generating `secp256k1` keys. +- [privval] \#5603 Add `--key` to `init`, `gen_validator`, `testnet` & `unsafe_reset_priv_validator` for use in generating `secp256k1` keys. ### BUG FIXES - [types] \#5523 Change json naming of `PartSetHeader` within `BlockID` from `parts` to `part_set_header` (@marbar3778) +- [privval] \#5638 Increase read/write timeout to 5s and calculate ping interval based on it (@JoeKash) diff --git a/privval/signer_dialer_endpoint.go b/privval/signer_dialer_endpoint.go index bd98314b6..93d26b043 100644 --- a/privval/signer_dialer_endpoint.go +++ b/privval/signer_dialer_endpoint.go @@ -15,24 +15,26 @@ const ( // SignerServiceEndpointOption sets an optional parameter on the SignerDialerEndpoint. type SignerServiceEndpointOption func(*SignerDialerEndpoint) -// SignerDialerEndpointTimeoutReadWrite sets the read and write timeout for connections -// from external signing processes. +// SignerDialerEndpointTimeoutReadWrite sets the read and write timeout for +// connections from client processes. func SignerDialerEndpointTimeoutReadWrite(timeout time.Duration) SignerServiceEndpointOption { return func(ss *SignerDialerEndpoint) { ss.timeoutReadWrite = timeout } } -// SignerDialerEndpointConnRetries sets the amount of attempted retries to acceptNewConnection. +// SignerDialerEndpointConnRetries sets the amount of attempted retries to +// acceptNewConnection. func SignerDialerEndpointConnRetries(retries int) SignerServiceEndpointOption { return func(ss *SignerDialerEndpoint) { ss.maxConnRetries = retries } } -// SignerDialerEndpointRetryWaitInterval sets the retry wait interval to a custom value +// SignerDialerEndpointRetryWaitInterval sets the retry wait interval to a +// custom value. func SignerDialerEndpointRetryWaitInterval(interval time.Duration) SignerServiceEndpointOption { return func(ss *SignerDialerEndpoint) { ss.retryWait = interval } } -// SignerDialerEndpoint dials using its dialer and responds to any -// signature requests using its privVal. +// SignerDialerEndpoint dials using its dialer and responds to any signature +// requests using its privVal. type SignerDialerEndpoint struct { signerEndpoint @@ -57,13 +59,13 @@ func NewSignerDialerEndpoint( maxConnRetries: defaultMaxDialRetries, } + sd.BaseService = *service.NewBaseService(logger, "SignerDialerEndpoint", sd) + sd.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second + for _, optionFunc := range options { optionFunc(sd) } - sd.BaseService = *service.NewBaseService(logger, "SignerDialerEndpoint", sd) - sd.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second - return sd } diff --git a/privval/signer_endpoint.go b/privval/signer_endpoint.go index 8ea5e2f8a..eb2ed442f 100644 --- a/privval/signer_endpoint.go +++ b/privval/signer_endpoint.go @@ -12,7 +12,7 @@ import ( ) const ( - defaultTimeoutReadWriteSeconds = 3 + defaultTimeoutReadWriteSeconds = 5 ) type signerEndpoint struct { diff --git a/privval/signer_listener_endpoint.go b/privval/signer_listener_endpoint.go index 1f05995fa..be5acb278 100644 --- a/privval/signer_listener_endpoint.go +++ b/privval/signer_listener_endpoint.go @@ -11,11 +11,22 @@ import ( privvalproto "github.com/tendermint/tendermint/proto/tendermint/privval" ) -// SignerValidatorEndpointOption sets an optional parameter on the SocketVal. -type SignerValidatorEndpointOption func(*SignerListenerEndpoint) +// SignerListenerEndpointOption sets an optional parameter on the SignerListenerEndpoint. +type SignerListenerEndpointOption func(*SignerListenerEndpoint) + +// SignerListenerEndpointTimeoutReadWrite sets the read and write timeout for +// connections from external signing processes. +// +// Default: 5s +func SignerListenerEndpointTimeoutReadWrite(timeout time.Duration) SignerListenerEndpointOption { + return func(sl *SignerListenerEndpoint) { sl.signerEndpoint.timeoutReadWrite = timeout } +} -// SignerListenerEndpoint listens for an external process to dial in -// and keeps the connection alive by dropping and reconnecting +// SignerListenerEndpoint listens for an external process to dial in and keeps +// the connection alive by dropping and reconnecting. +// +// The process will send pings every ~3s (read/write timeout * 2/3) to keep the +// connection alive. type SignerListenerEndpoint struct { signerEndpoint @@ -25,6 +36,7 @@ type SignerListenerEndpoint struct { timeoutAccept time.Duration pingTimer *time.Ticker + pingInterval time.Duration instanceMtx tmsync.Mutex // Ensures instance public methods access, i.e. SendRequest } @@ -33,15 +45,21 @@ type SignerListenerEndpoint struct { func NewSignerListenerEndpoint( logger log.Logger, listener net.Listener, + options ...SignerListenerEndpointOption, ) *SignerListenerEndpoint { - sc := &SignerListenerEndpoint{ + sl := &SignerListenerEndpoint{ listener: listener, timeoutAccept: defaultTimeoutAcceptSeconds * time.Second, } - sc.BaseService = *service.NewBaseService(logger, "SignerListenerEndpoint", sc) - sc.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second - return sc + sl.BaseService = *service.NewBaseService(logger, "SignerListenerEndpoint", sl) + sl.signerEndpoint.timeoutReadWrite = defaultTimeoutReadWriteSeconds * time.Second + + for _, optionFunc := range options { + optionFunc(sl) + } + + return sl } // OnStart implements service.Service. @@ -49,7 +67,9 @@ func (sl *SignerListenerEndpoint) OnStart() error { sl.connectRequestCh = make(chan struct{}) sl.connectionAvailableCh = make(chan net.Conn) - sl.pingTimer = time.NewTicker(defaultPingPeriodMilliseconds * time.Millisecond) + // NOTE: ping timeout must be less than read/write timeout + sl.pingInterval = time.Duration(sl.signerEndpoint.timeoutReadWrite.Milliseconds()*2/3) * time.Millisecond + sl.pingTimer = time.NewTicker(sl.pingInterval) go sl.serviceLoop() go sl.pingLoop() @@ -117,6 +137,7 @@ func (sl *SignerListenerEndpoint) ensureConnection(maxWait time.Duration) error } // block until connected or timeout + sl.Logger.Info("SignerListener: Blocking for connection") sl.triggerConnect() err := sl.WaitConnection(sl.connectionAvailableCh, maxWait) if err != nil { diff --git a/privval/signer_listener_endpoint_test.go b/privval/signer_listener_endpoint_test.go index 02b19eff3..cbd45e6ce 100644 --- a/privval/signer_listener_endpoint_test.go +++ b/privval/signer_listener_endpoint_test.go @@ -168,7 +168,11 @@ func newSignerListenerEndpoint(logger log.Logger, addr string, timeoutReadWrite listener = tcpLn } - return NewSignerListenerEndpoint(logger, listener) + return NewSignerListenerEndpoint( + logger, + listener, + SignerListenerEndpointTimeoutReadWrite(testTimeoutReadWrite), + ) } func startListenerEndpointAsync(t *testing.T, sle *SignerListenerEndpoint, endpointIsOpenCh chan struct{}) { diff --git a/privval/socket_listeners.go b/privval/socket_listeners.go index 908e05e2e..ad49adac4 100644 --- a/privval/socket_listeners.go +++ b/privval/socket_listeners.go @@ -9,8 +9,7 @@ import ( ) const ( - defaultTimeoutAcceptSeconds = 3 - defaultPingPeriodMilliseconds = 100 + defaultTimeoutAcceptSeconds = 3 ) // timeoutError can be used to check if an error returned from the netp package