From 3155549513256564eb3aa3b77bb05d5aa35abfd1 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Thu, 11 Feb 2021 14:57:14 +0100
Subject: [PATCH] fix(http body saver): properly deal with EOF terminated
 bodies (#226)

See https://github.com/ooni/probe-engine/issues/1191
---
 internal/engine/netx/httptransport/saver.go   | 18 +++++++
 .../netx/httptransport/saver_internal_test.go | 51 +++++++++++++++++++
 2 files changed, 69 insertions(+)
 create mode 100644 internal/engine/netx/httptransport/saver_internal_test.go

diff --git a/internal/engine/netx/httptransport/saver.go b/internal/engine/netx/httptransport/saver.go
index 25a869d..f75bfdb 100644
--- a/internal/engine/netx/httptransport/saver.go
+++ b/internal/engine/netx/httptransport/saver.go
@@ -2,6 +2,7 @@ package httptransport
 
 import (
 	"bytes"
+	"errors"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -125,6 +126,7 @@ func (txp SaverBodyHTTPTransport) RoundTrip(req *http.Request) (*http.Response,
 		return nil, err
 	}
 	data, err := saverSnapRead(resp.Body, snapsize)
+	err = ignoreExpectedEOF(err, resp)
 	if err != nil {
 		resp.Body.Close()
 		return nil, err
@@ -139,6 +141,22 @@ func (txp SaverBodyHTTPTransport) RoundTrip(req *http.Request) (*http.Response,
 	return resp, nil
 }
 
+// ignoreExpectedEOF converts an error signalling the end of the body
+// into a success. We know that we are in such condition when the
+// resp.Close hint flag is set to true. (Thanks, stdlib!)
+//
+// See https://github.com/ooni/probe-engine/issues/1191 for an analysis
+// of how this error was impacting measurements and data quality.
+func ignoreExpectedEOF(err error, resp *http.Response) error {
+	if err == nil {
+		return nil
+	}
+	if errors.Is(err, io.EOF) && resp.Close {
+		return nil
+	}
+	return err
+}
+
 func saverSnapRead(r io.ReadCloser, snapsize int) ([]byte, error) {
 	return ioutil.ReadAll(io.LimitReader(r, int64(snapsize)))
 }
diff --git a/internal/engine/netx/httptransport/saver_internal_test.go b/internal/engine/netx/httptransport/saver_internal_test.go
new file mode 100644
index 0000000..5777d49
--- /dev/null
+++ b/internal/engine/netx/httptransport/saver_internal_test.go
@@ -0,0 +1,51 @@
+package httptransport
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"net/http"
+	"testing"
+)
+
+func composeWithEOFError(msg string) error {
+	return fmt.Errorf("%w: %s", io.EOF, msg)
+}
+
+func TestIgnoreExpectedEOFWithNoError(t *testing.T) {
+	if err := ignoreExpectedEOF(nil, nil); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestIgnoreExpectedEOFWithEOFErrorButNoCloseHint(t *testing.T) {
+	resp := &http.Response{}
+	in := composeWithEOFError("antani")
+	if err := ignoreExpectedEOF(in, resp); !errors.Is(err, io.EOF) {
+		t.Fatalf("not the error we expected: %+v", err)
+	}
+}
+
+func TestIgnoreExpectedEOFWithEOFErrorAndCloseHint(t *testing.T) {
+	resp := &http.Response{Close: true}
+	in := composeWithEOFError("antani")
+	if err := ignoreExpectedEOF(in, resp); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestIgnoreExpectedEOFAnyOtherErrorAndCloseHint(t *testing.T) {
+	resp := &http.Response{Close: true}
+	in := errors.New("antani")
+	if err := ignoreExpectedEOF(in, resp); !errors.Is(err, in) {
+		t.Fatalf("not the error we expected: %+v", err)
+	}
+}
+
+func TestIgnoreExpectedEOFAnyOtherErrorAndNoCloseHint(t *testing.T) {
+	resp := &http.Response{Close: false /*explicit*/}
+	in := errors.New("antani")
+	if err := ignoreExpectedEOF(in, resp); !errors.Is(err, in) {
+		t.Fatalf("not the error we expected: %+v", err)
+	}
+}