From ebed1927bfb1e03bf92a83d5c1d10a6dad775885 Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Mon, 9 May 2022 18:50:12 +0200 Subject: [PATCH 1/4] rpcclient: fix formatting --- rpcclient/infrastructure.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/rpcclient/infrastructure.go b/rpcclient/infrastructure.go index 63874c1b4e..22430a2e1c 100644 --- a/rpcclient/infrastructure.go +++ b/rpcclient/infrastructure.go @@ -768,11 +768,14 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest) { } url := protocol + "://" + c.config.Host - var err error - var backoff time.Duration - var httpResponse *http.Response + var ( + err error + backoff time.Duration + httpResponse *http.Response + ) + tries := 10 - for i := 0; tries == 0 || i < tries; i++ { + for i := 0; i < tries; i++ { bodyReader := bytes.NewReader(jReq.marshalledJSON) httpReq, err := http.NewRequest("POST", url, bodyReader) if err != nil { @@ -799,7 +802,9 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest) { if backoff > time.Minute { backoff = time.Minute } - log.Debugf("Failed command [%s] with id %d attempt %d. Retrying in %v... \n", jReq.method, jReq.id, i, backoff) + log.Debugf("Failed command [%s] with id %d attempt %d."+ + " Retrying in %v... \n", jReq.method, jReq.id, + i, backoff) time.Sleep(backoff) continue } From 966511246dd03b9bf88fd6607e2449ea0cfd522a Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Mon, 9 May 2022 18:51:22 +0200 Subject: [PATCH 2/4] rpclient: fix masked error causing crash after max retries This commit fixes the error that is masked inside the for loop's scope. Previously after max retries the error didn't leave the for scope and therefore httpResponse remained nil which in turn resulted in a crash. --- rpcclient/infrastructure.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/rpcclient/infrastructure.go b/rpcclient/infrastructure.go index 22430a2e1c..93b036c0d6 100644 --- a/rpcclient/infrastructure.go +++ b/rpcclient/infrastructure.go @@ -776,8 +776,10 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest) { tries := 10 for i := 0; i < tries; i++ { + var httpReq *http.Request + bodyReader := bytes.NewReader(jReq.marshalledJSON) - httpReq, err := http.NewRequest("POST", url, bodyReader) + httpReq, err = http.NewRequest("POST", url, bodyReader) if err != nil { jReq.responseChan <- &Response{result: nil, err: err} return @@ -815,6 +817,15 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest) { return } + // We still want to return an error if for any reason the respone + // remains empty. + if httpResponse == nil { + jReq.responseChan <- &Response{ + err: fmt.Errorf("invalid http POST response (nil), "+ + "method: %s, id: %d", jReq.method, jReq.id), + } + } + // Read the raw bytes and close the response. respBytes, err := ioutil.ReadAll(httpResponse.Body) httpResponse.Body.Close() From 9babf1fa08364e4a58f1b222e5ef79c324070632 Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Mon, 9 May 2022 18:58:49 +0200 Subject: [PATCH 3/4] rpcclient: fix backoff logic This commit removes Sleep() from the rety handler so that the shutdown request is always respected. Furthermore the maximum retry count is corrected. --- rpcclient/infrastructure.go | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/rpcclient/infrastructure.go b/rpcclient/infrastructure.go index 93b036c0d6..b7732bc6c5 100644 --- a/rpcclient/infrastructure.go +++ b/rpcclient/infrastructure.go @@ -761,7 +761,9 @@ out: // handleSendPostMessage handles performing the passed HTTP request, reading the // result, unmarshalling it, and delivering the unmarshalled result to the // provided response channel. -func (c *Client) handleSendPostMessage(jReq *jsonRequest) { +func (c *Client) handleSendPostMessage(jReq *jsonRequest, + shutdown chan struct{}) { + protocol := "http" if !c.config.DisableTLS { protocol = "https" @@ -799,18 +801,27 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest) { httpReq.SetBasicAuth(user, pass) httpResponse, err = c.httpClient.Do(httpReq) - if err != nil { - backoff = requestRetryInterval * time.Duration(i+1) - if backoff > time.Minute { - backoff = time.Minute - } - log.Debugf("Failed command [%s] with id %d attempt %d."+ - " Retrying in %v... \n", jReq.method, jReq.id, - i, backoff) - time.Sleep(backoff) - continue + + // Quit the retry loop on success or if we can't retry anymore. + if err == nil || i == tries-1 { + break + } + + // Backoff sleep otherwise. + backoff = requestRetryInterval * time.Duration(i+1) + if backoff > time.Minute { + backoff = time.Minute + } + log.Debugf("Failed command [%s] with id %d attempt %d."+ + " Retrying in %v... \n", jReq.method, jReq.id, + i, backoff) + + select { + case <-time.After(backoff): + + case <-shutdown: + return } - break } if err != nil { jReq.responseChan <- &Response{err: err} @@ -874,7 +885,7 @@ out: // is closed. select { case jReq := <-c.sendPostChan: - c.handleSendPostMessage(jReq) + c.handleSendPostMessage(jReq, c.shutdown) case <-c.shutdown: break out From 97313ac873eee4365fc1de253a538335defcfed7 Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Thu, 12 May 2022 22:09:38 +0200 Subject: [PATCH 4/4] rpcclient: save the last error when retrying --- rpcclient/infrastructure.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/rpcclient/infrastructure.go b/rpcclient/infrastructure.go index b7732bc6c5..f931b83501 100644 --- a/rpcclient/infrastructure.go +++ b/rpcclient/infrastructure.go @@ -771,7 +771,7 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest, url := protocol + "://" + c.config.Host var ( - err error + err, lastErr error backoff time.Duration httpResponse *http.Response ) @@ -807,6 +807,12 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest, break } + // Save the last error for the case where we backoff further, + // retry and get an invalid response but no error. If this + // happens the saved last error will be used to enrich the error + // message that we pass back to the caller. + lastErr = err + // Backoff sleep otherwise. backoff = requestRetryInterval * time.Duration(i+1) if backoff > time.Minute { @@ -833,7 +839,8 @@ func (c *Client) handleSendPostMessage(jReq *jsonRequest, if httpResponse == nil { jReq.responseChan <- &Response{ err: fmt.Errorf("invalid http POST response (nil), "+ - "method: %s, id: %d", jReq.method, jReq.id), + "method: %s, id: %d, last error=%v", + jReq.method, jReq.id, lastErr), } }