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: resp.Body should be closed when error occurred during redirection #19976

Closed
Tevic opened this issue Apr 14, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@Tevic
Copy link
Contributor

commented Apr 14, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8.1 linux/amd64

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

GOARCH="amd64"
GOOS="linux"

What did you do?

Write a server return 302 without specify "Location" header. Requests to the server will get error like "Get http://127.0.0.1:3737: 302 response missing Location header", then write a client do requests to the server.

package main

import (
	"encoding/base64"
	"encoding/binary"
	"log"
	"net/http"
	"time"
)

func MockServer() {

	http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
		w.WriteHeader(302)
		if wf, ok := w.(http.Flusher); ok {
			wf.Flush()
		}
		if wh, ok := w.(http.Hijacker); ok {
			conn, _, err := wh.Hijack()
			if err != nil {
				log.Println(err)
			} else {
				conn.Close()
			}
		}
	})
	log.Fatal(http.ListenAndServe(":3737", nil))
}

func main() {
	go MockServer()
	req, err := http.NewRequest("GET", "http://127.0.0.1:3737", nil)
	if err != nil {
		log.Fatal(err)
	}
	for i := 1; i < 6000; i++ {
		cli := http.Client{}
		_, err := cli.Do(req)
		if err != nil {
			log.Println(err)
		}
	}
	time.Sleep(time.Hour)
}

What did you expect to see?

no filehandler leaks

What did you see instead?

filehandler leaks
lsof -np PID get connections whose state are CLOSE_WAIT:

Test    92983 tevic 6000u     IPv4 14443078      0t0     TCP 127.0.0.1:52806->127.0.0.1:3737 (CLOSE_WAIT)
Test    92983 tevic 6001u     IPv4 14439976      0t0     TCP 127.0.0.1:52809->127.0.0.1:3737 (CLOSE_WAIT)
Test    92983 tevic 6002u     IPv4 14443079      0t0     TCP 127.0.0.1:52810->127.0.0.1:3737 (CLOSE_WAIT)

After a while, these connections becomes "protocol: TCP" on Ubuntu 16.04.2 LTS, and can not be recycled by os.

Test    92983 tevic 6000u     sock      0,8      0t0 14443078 protocol: TCP
Test    92983 tevic 6001u     sock      0,8      0t0 14439976 protocol: TCP
Test    92983 tevic 6002u     sock      0,8      0t0 14443079 protocol: TCP

On Ubuntu 14.04.5 LTS,it becomes "can't identify protocol".

Difference between go1.6 and go1.7+

go1.6.4: net/http/client.go
Line 492: resp.Body was closed.

485		if shouldRedirect(resp.StatusCode) {
486			// Read the body if small so underlying TCP connection will be re-used.
487			// No need to check for errors: if it fails, Transport won't reuse it anyway.
488			const maxBodySlurpSize = 2 << 10
489			if resp.ContentLength == -1 || resp.ContentLength <= maxBodySlurpSize {
490				io.CopyN(ioutil.Discard, resp.Body, maxBodySlurpSize)
491			}
492			resp.Body.Close()
493			if urlStr = resp.Header.Get("Location"); urlStr == "" {
494				err = fmt.Errorf("%d response missing Location header", resp.StatusCode)
495				break
496			}
497			base = req.URL
498			via = append(via, req)
499			continue
500		}
501		return resp, nil
502	}

go1.8.1: net/http/client.go
Line 527,530,545: resp.Body will never close.

525			loc := resp.Header.Get("Location")
526			if loc == "" {
527				return nil, uerr(fmt.Errorf("%d response missing Location header", resp.StatusCode))
528			}
529			u, err := req.URL.Parse(loc)
530			if err != nil {
531				return nil, uerr(fmt.Errorf("failed to parse Location header %q: %v", loc, err))
532			}
533			ireq := reqs[0]
534			req = &Request{
535				Method:   redirectMethod,
536				Response: resp,
537				URL:      u,
538				Header:   make(Header),
539				Cancel:   ireq.Cancel,
540				ctx:      ireq.ctx,
541			}
542			if includeBody && ireq.GetBody != nil {
543				req.Body, err = ireq.GetBody()
544				if err != nil {
545					return nil, uerr(err)
546				}
547				req.ContentLength = ireq.ContentLength
548			}

@Tevic Tevic changed the title net/http: resp.Body should close when error occurred during redirection net/http: resp.Body should be closed when error occurred during redirection Apr 14, 2017

@bradfitz bradfitz added this to the Go1.9 milestone Apr 15, 2017

@dividinglimits

This comment has been minimized.

Copy link

commented Apr 17, 2017

I will have a CL prepared by the end of the week (and corresponding signed CLA)

@Tevic

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

@dividinglimits @bradfitz I've a CL prepared ,signed with CLA ,when use 'git codereview mail', it always get error:

fatal: remote error: Access Denied (not available from your location)
(running: git push -q origin HEAD:refs/for/master)
git-codereview: exit status 128
@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

not available from your location

Hm, weird. It's possible that we can't accept contributions from some countries. Email me privately (bradfitz golang) if you want to debug.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 18, 2017

CL https://golang.org/cl/40940 mentions this issue.

@gopherbot gopherbot closed this in e51e0f9 Apr 27, 2017

@golang golang locked and limited conversation to collaborators Apr 27, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.