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: method decHostConnCount should not dec counter when somebody is block on connPerHostAvailable #29889

Closed
j2gg0s opened this issue Jan 23, 2019 · 6 comments

Comments

@j2gg0s
Copy link

@j2gg0s j2gg0s commented Jan 23, 2019

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

$ go version
go version go1.11.3 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env

What did you do?

About net/http/transport.getConn
When current conn number is greater than MaxConnPerHost, incHostConnCount return connPerHostAvailable and caller block on this chan, but connPerHostCount will not inc.
When somebody is block on connPerHostAvailable and one conn be closed, decHostConnCount notify and connPerHostCount dec.

One Example:

  • MaxConnsPerHost is one

Time 1

  • ReqA call getConn and get connA, connPerHostCount now is one

Time 2

  • ReqB call getConn but block on connPerHostAvailable or idleConnCh

Time 3

  • ReqA close connA, notify ReqB and dec connPerHostCount
  • ReqB notify by idleConnCh and create connB

Time 4

  • ReqC call getConn and create connC

current two conns exist, but MaxConnsPerHost is one.

What did you expect to see?

When somebody is block on connPerHostAvailable and one conn be closed, decHostConnCount notify and connPerHostCount don't dec.

@FiloSottile FiloSottile changed the title method decHostConnCount should not dec counter when somebody is block on connPerHostAvailable net/http: method decHostConnCount should not dec counter when somebody is block on connPerHostAvailable Jan 23, 2019
@FiloSottile FiloSottile modified the milestones: Gccgo, Go1.13 Jan 23, 2019
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jan 23, 2019

/cc @bradfitz

@j2gg0s
Copy link
Author

@j2gg0s j2gg0s commented Jan 24, 2019

Add one unit test to explain what I want to express.
#29910

@billybanfield
Copy link

@billybanfield billybanfield commented Feb 19, 2019

Hey all! We appear to have run into this issue as well. It manifested as MaxConnsPerHost not being respected if concurrently made requests failed and then begin to succeed. Here's a repro demonstrating this behavior: https://github.com/billybanfield/tcpbug/blob/master/tcpbug.go. Happy to provide any additional info needed here.

Moving the decrement for connPerHostCount in transport.go as suggested by the original reporter fixes the issue.

@j2gg0s
Copy link
Author

@j2gg0s j2gg0s commented Feb 26, 2019

@bradfitz @FiloSottile Need some feedback.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 28, 2019

MaxConnsPerHost is fairly broken; see #32336. I will try to keep this bug in mind when I develop a fix for the other. Maybe it will be easy to fix both.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 29, 2019

Change https://golang.org/cl/184262 mentions this issue: net/http: fix Transport.MaxConnsPerHost limits & idle pool races

@gopherbot gopherbot closed this in fbaf881 Jul 8, 2019
@golang golang locked and limited conversation to collaborators Jul 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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