Repository: beatlabs/patron
Pull request #334 (2021-01-05 14:47:49)

Log 'connection reset' errors on info level
<!--
Thanks for taking precious time for making a PR.

Before creating a pull request, please make sure:
- Your PR solves one problem for which a issue exist and a solution has been discussed
- You have read the guide for contributing
  - See https://github.com/beatlabs/patron/blob/master/README.md#how-to-contribute
- You signed all your commits (otherwise we won't be able to merge the PR)
  - See https://github.com/beatlabs/patron/blob/master/SIGNYOURWORK.md
- You added unit tests for the new functionality
- You mention in the PR description which issue it is addressing, e.g. "Resolves #123"
-->

## Which problem is this PR solving?

Solves https://github.com/beatlabs/patron/issues/332
_(I know the issue hasn't been discussed, but the code was ready anyway)_

## Short description of the changes

When an error happens in compression middleware which is not really an error but rather an expected (although rare) thing, don't spoil the logs with an error message.
The implementation is based on what AWS considers to belong to that kind of errors.


codecov[bot]: # [Codecov](https://codecov.io/gh/beatlabs/patron/pull/334?src=pr&el=h1) Report
> Merging [#334](https://codecov.io/gh/beatlabs/patron/pull/334?src=pr&el=desc) (5fabe90) into [master](https://codecov.io/gh/beatlabs/patron/commit/2a2b4d2e99a3bc6e452ff7e6d69c5c577942b244?el=desc) (2a2b4d2) will **increase** coverage by `0.02%`.
> The diff coverage is `58.33%`.

[![Impacted file tree graph](https://codecov.io/gh/beatlabs/patron/pull/334/graphs/tree.svg?width=650&height=150&src=pr&token=sxY15rXW1X)](https://codecov.io/gh/beatlabs/patron/pull/334?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   73.89%   73.92%   +0.02%     
==========================================
  Files          64       64              
  Lines        3590     3601      +11     
==========================================
+ Hits         2653     2662       +9     
- Misses        835      837       +2     
  Partials      102      102              
```


| [Impacted Files](https://codecov.io/gh/beatlabs/patron/pull/334?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [component/http/middleware.go](https://codecov.io/gh/beatlabs/patron/pull/334/diff?src=pr&el=tree#diff-Y29tcG9uZW50L2h0dHAvbWlkZGxld2FyZS5nbw==) | `87.12% <58.33%> (-1.36%)` | :arrow_down: |
| [component/async/sqs/sqs.go](https://codecov.io/gh/beatlabs/patron/pull/334/diff?src=pr&el=tree#diff-Y29tcG9uZW50L2FzeW5jL3Nxcy9zcXMuZ28=) | `79.08% <0.00%> (+1.02%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/beatlabs/patron/pull/334?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/beatlabs/patron/pull/334?src=pr&el=footer). Last update [2a2b4d2...5fabe90](https://codecov.io/gh/beatlabs/patron/pull/334?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/component/http/middleware.go b/component/http/middleware.go
index 0c43d12d1..0f017c581 100644
--- a/component/http/middleware.go
+++ b/component/http/middleware.go
@@ -279,7 +279,12 @@ func NewCompressionMiddleware(deflateLevel int, ignoreRoutes ...string) Middlewa
 			defer func(cw io.WriteCloser) {
 				err := cw.Close()
 				if err != nil {
-					log.Errorf("error in deferred call to Close() method on %v compression middleware : %v", hdr, err.Error())
+					msgErr := fmt.Sprintf("error in deferred call to Close() method on %v compression middleware : %v", hdr, err.Error())
+					if isErrConnectionReset(err) {
+						log.Info(msgErr)
+					} else {
+						log.Error(msgErr)
+					}
 				}
 			}(writer)
 
@@ -290,6 +295,27 @@ func NewCompressionMiddleware(deflateLevel int, ignoreRoutes ...string) Middlewa
 	}
 }
 
+// isErrConnectionReset detects if an error has happened due to a connection reset, broken pipe or similar.
+// Implementation is copied from AWS SDK, package request. We assume that it is a complete genuine implementation.
+func isErrConnectionReset(err error) bool {
+	errMsg := err.Error()
+
+	// See the explanation here: https://github.com/aws/aws-sdk-go/issues/2525#issuecomment-519263830
+	// It is a little bit vague, but it seems they mean that this specific error happens when we stopped reading for some reason
+	// even though there was something to read. This might have happened due to a wrong length header for example.
+	// So it might've been our error, not an error of remote server when it closes connection unexpectedly.
+	if strings.Contains(errMsg, "read: connection reset") {
+		return false
+	}
+
+	if strings.Contains(errMsg, "connection reset") ||
+		strings.Contains(errMsg, "broken pipe") {
+		return true
+	}
+
+	return false
+}
+
 // Write provides write func to the writer.
 func (w compressionResponseWriter) Write(b []byte) (int, error) {
 	return w.Writer.Write(b)


Test patch:
diff --git a/component/http/middleware_test.go b/component/http/middleware_test.go
index 3a1d17d0b..5943c76f7 100644
--- a/component/http/middleware_test.go
+++ b/component/http/middleware_test.go
@@ -551,3 +551,37 @@ func TestAddWithWeight(t *testing.T) {
 		})
 	}
 }
+
+func TestIsConnectionReset(t *testing.T) {
+	tests := map[string]struct {
+		err      error
+		expected bool
+	}{
+		"Broken pipe": {
+			err:      errors.New("blah blah broken pipe blah blah"),
+			expected: true,
+		},
+		"connection reset": {
+			err:      errors.New("blah blah connection reset blah blah"),
+			expected: true,
+		},
+		"read: connection reset": {
+			err:      errors.New("blah blah read: connection reset blah blah"),
+			expected: false,
+		},
+		"b00m random error": {
+			err:      errors.New("blah blah b00m random error blah blah"),
+			expected: false,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			// when
+			result := isErrConnectionReset(tc.err)
+
+			// then
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}