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

Jump to bottom

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
Closed

net/http: method decHostConnCount should not dec counter when somebody is block on connPerHostAvailable #29889

j2gg0s opened this issue Jan 23, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.13

Comments

@j2gg0s
Copy link
Contributor

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 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 23, 2019
@FiloSottile FiloSottile modified the milestones: Gccgo, Go1.13 Jan 23, 2019
@FiloSottile
Copy link
Contributor

/cc @bradfitz

@j2gg0s
Copy link
Contributor Author

j2gg0s commented Jan 24, 2019

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

@williambanfield
Copy link

williambanfield commented Feb 19, 2019 •
edited

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
Contributor Author

j2gg0s commented Feb 26, 2019

@bradfitz @FiloSottile Need some feedback.

@rsc
Copy link
Contributor

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.

@rsc rsc mentioned this issue Jun 28, 2019
Closed
@gopherbot
Copy link

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

@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
5 participants
@rsc @FiloSottile @williambanfield @gopherbot @j2gg0s