Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: ResponseHeaderTimeout not honoured - response from server not visible to implementing part #40926

Open
jakub-bt-vs opened this issue Aug 20, 2020 · 2 comments

Comments

@jakub-bt-vs
Copy link

@jakub-bt-vs jakub-bt-vs commented Aug 20, 2020

What version of Go are you using (go version)?

$ go version
go version go1.15 windows/amd64

Does this issue reproduce with the latest release?

Yes it does

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\xxx\AppData\Local\go-build
set GOENV=C:\Users\xxx\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\xxx\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\xxx\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\xxx\AppData\Local\Temp\go-build406563982=/tmp/go-build -gno-record-gcc-switches

What did you do?

The property ResponseHeaderTimeout in http.Transport -is not honoured.
HTTP client performing POST/PUT requests is not informed about server response.

According to documentation if ResponseHeaderTimeout is set and fully writing the request happens implementation shall wait for specified time for response before sending error.

Configure ResponseHeaderTimeout
Prepare a POST/PUT request with body.
Try to send it to the server requiring authentication.
The server respond with authentication request and closed the client connection (to avoid sending huge file from not authenticated source)

	import (
	"bufio"
	"bytes"
	"crypto/rand"
	"fmt"
	"io"
	"io/ioutil"
	"net"
	"net/http"
	"strconv"
	"strings"
	"testing"
	"time"
)

func newLocalListener(t *testing.T) net.Listener {
	ln, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		ln, err = net.Listen("tcp6", "[::1]:0")
	}
	if err != nil {
		t.Fatal(err)
	}
	return ln
}

const Issue40926Body = "<HTML><HEAD><TITLE>401 Authorization Required</TITLE></HEAD><BODY></BODY></HTML>\r\n"
const Issue40926Resp = "HTTP/1.0 401 Unauthorized\r\n" +
	"WWW-Authenticate: Basic realm=\"GoTest\"\r\n" +
	"\r\n" +
	Issue40926Body

var issue40926TestCases = []struct {
	closeAfterHeaders     bool
	responseHeaderTimeout time.Duration
}{
	{true, time.Second},
	{false, 0},
}

func Issue40926ContentHandling(t *testing.T, listener net.Listener, listenerDone chan struct{}, closeAfterHeaders bool) {
	defer close(listenerDone)
	c, err := listener.Accept()
	if err != nil {
		t.Errorf("Accept: %v", err)
		return
	}
	defer c.Close()
	reqReader := bufio.NewReader(c)
	//Read data from request
	req := make([]byte, 1024)
	var total int64
	var contentLength int64
	isHeader := true

	for {
		var readError error
		if isHeader {
			header, err := reqReader.ReadString('\n')
			if err == nil && (header == "\r" || header == "\r\n" || header == "\n" || header == "") {
				isHeader = false
			}
			if strings.HasPrefix(header, "Content-Length: ") {
				header = header[16 : len(header)-2]
				contentLength, _ = strconv.ParseInt(header, 10, 32)
			}
			readError = err
		} else {
			n, err := reqReader.Read(req)
			total += int64(n)
			if err == io.EOF || n == 0 || contentLength == total {
				break
			}
			readError = err
		}

		if !isHeader && closeAfterHeaders { //After any data is read - close connection
			break
		}
		if readError != nil {
			t.Errorf("Reading from req failed %s", err.Error())
			return
		}
	}
	c.Write([]byte(Issue40926Resp))
	c.Close()
}

func funcIssue40926ResponseHeaderTimeout(t *testing.T, closeAfterHeaders bool, responseHeaderTimeout time.Duration) {
	listener := newLocalListener(t)
	defer listener.Close()
	listenerDone := make(chan struct{})
	go Issue40926ContentHandling(t, listener, listenerDone, closeAfterHeaders)

	//After connection is closed client shall wait for 5 seconds for response
	c := &http.Client{
		Transport: &http.Transport{
			ResponseHeaderTimeout: responseHeaderTimeout,
		},
	}
	//Keep the upload size big engought
	body := make([]byte, 1024*1024*10)
	rand.Read(body)

	req, err := http.NewRequest("POST", fmt.Sprintf("http://%s/", listener.Addr().String()), bytes.NewReader(body))
	req.ContentLength = int64(len(body))
	if err != nil {
		t.Fatal(err)
	}
	resp, err := c.Do(req)

	if err != nil {
		t.Fatal(err)
	}
	if resp == nil {
		t.Fatal("Response expected")
	}
	body, err = ioutil.ReadAll(resp.Body)
	resp.Body.Close()
	bodyText := string(body)
	if strings.Compare(Issue40926Body, bodyText) != 0 {
		t.Fatal("Response check failed")
	}

	// Wait unconditionally for the listener goroutine to exit
	<-listenerDone
}

func TestIssue40926ResponseHeaderTimeout(t *testing.T) {
	for _, tc := range issue40926TestCases {
		t.Run(fmt.Sprintf("Close connection after receiving headers: %t, ResponseHeaderTimeout: %d", tc.closeAfterHeaders, tc.responseHeaderTimeout), func(t *testing.T) {
			funcIssue40926ResponseHeaderTimeout(t, tc.closeAfterHeaders, tc.responseHeaderTimeout)
		})
	}
}

What did you expect to see?

Correct response as server sends is before closing connection.
According to RFC that is right way of handling file uploads - RFC 2616:

The server is refusing to process a request because the request entity is larger than the server is willing or able to process. The server MAY close the connection to prevent the client from continuing the request.

Additionally in same RFC (2616)
8.2.4 Client Behavior if Server Prematurely Closes Connection

If an HTTP/1.1 client sends a request which includes a request body, but which does not include an Expect request-header field with the "100-continue" expectation, and if the client is not directly connected to an HTTP/1.1 origin server, and if the client sees the connection close before receiving any status from the server, the client SHOULD retry the request.

In case server is closing connection, client implementation will face write error, that is fine according to RFC it is up to client to retry. But if before closing of the connection response is being send it shall be visible to the client.

What did you see instead?

Error from http.Transport on write of request body should not be returned if within time specified in http.Transport property ResponseHeaderTimeout a valid response is received.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 20, 2020

Change https://golang.org/cl/249181 mentions this issue: Fixes #40926 for HTTP response on write error.

jakub-bt-vs added a commit to jakub-bt-vs/go that referenced this issue Aug 20, 2020
@ALTree ALTree changed the title ResponseHeaderTimeout not honoured - response from server not visible to implementing part net/http: ResponseHeaderTimeout not honoured - response from server not visible to implementing part Aug 20, 2020
@ALTree ALTree added this to the Unplanned milestone Aug 20, 2020
@jakub-bt-vs
Copy link
Author

@jakub-bt-vs jakub-bt-vs commented Aug 21, 2020

@ALTree I have already provided fix for it in the PR #40926 can it be merged relatively quickly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.