Curious if you just tried to walk backwards through the connections?

Curious if you just tried to walk backwards through the connections?

@fraenkel
Could you elaborate your question?

@ochanism Your fix was to randomly start somewhere in the list. The current code walks forward. Since connections are added to the end, in most cases, the next connection with availability will be the last one. Obviously it all depends on which streams in which connections are closed.

Ultimately the bigger issue is that one has no way of configuring your new option from net/http. Now if this is only for use from net/http2 its less of an issue.

Its also related to #34511

@fraenkel
Client in net/http doesn't have this issue that I raised since the number of TCP connections for connection pooling can be adjusted via MaxIdleConns. However, there is no way to control the number of TCP connections in net/http2. So I've added MinConcurrentConns to control it.
Moreover, walking connections backward cannot resolve this issue. In this case, every request will select the last connection at a high probability and the remaining connections in the pool will be expired
due to IdleTimeout. So I've made the client randomly select a connection within MinConcurrentConns window to prevent the expiration.

Your selection will have similar issues in practice. Randomly selecting won't solve optimizing the network traffic across multiple connections. The underlying locking is a bit loose and writes currently lock reads. Which means depending on how loaded (# of streams) a connection is, and how much data is being sent back, the network may appear to be utilized but you are still dealing with lots of contention.

The fact that you are creating a new rand for each request does not provide any guarantees over the selection across the connections.

@fraenkel
Yes, you're right. All the random algorithms in real world have the same problem. The goal of my proposal is not to utilize network resource optimally. The main issue of the current http2 client is to use a small number of tcp connections even if there is a large traffic (not high frequent request). I can make it optimal, but I don't want to make it complex. I think the random algorithm shows a good performance for generic use cases. Of course, as you mentioned, there could be an issue for some edge cases.

/cc @bradfitz @tombergan

@cagedmantis @bradfitz @tombergan
Please give me any feedback.
I've been waiting for your review for 1 month.

It sounds like a knob we wouldn't want to expose to users.

How would you document how to tune it?

I'd rather just see it be automatically fast instead.

It sounds like a knob we wouldn't want to expose to users.

How would you document how to tune it?

I'd rather just see it be automatically fast instead.

@bradfitz
I agree with your opinion. It would be very nice if the value is adjusted automatically according to the current traffic pattern. But as you know, estimating and understanding the traffic patterns is not an easy problem. Many variables are related to it.

I think the MinConcurrentConns field has a similar role to the MaxIdleConns field in HTTP transport. (Not exact same, but similar)
Normally, most users don't need to care about MinConcurrentConns value. When MinConcurrentConns = 0, it will work as the previous. But some users will face the low-throughput problem like me, and they need to find an appropriate MinConcurrentConns value for their traffic pattern.

I've been running a server with this library in production, and there was a huge gain in terms of throughput. When I used the vanilla net/http library, an HTTP client uses only one or two TCP connections even if there is huge traffic. When I set strictMaxConcurrentStreams to false, I expected that the number of TCP connections would be increased if there is burst traffic. But it didn't work as I expected.

What do you think about this? Do you have any idea?

We've noticed this in rclone too rclone/rclone#3631 (comment)

When uploading 4 files at once to Google Drive

Without HTTP/2: 4 Connections (on netstat) & Total Speed: 320 MB/s
With HTTP/2: 1 Connection (on netstat) & Total Speed: 160 MB/s

So that is 50% of the speed using HTTP/2. My conjecture is that would go faster if we could persuade the HTTP/2 transport to use more TCP connections but I don't know how to do that.

Without HTTP/2: 4 Connections (on netstat) & Total Speed: 320 MB/s
With HTTP/2: 1 Connection (on netstat) & Total Speed: 160 MB/s

I can reproduce similar values with following snippet, by changing the DisableKeepAlives option, that allows to create multiple connections per request when is set to true

package main

import (
	"fmt"
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
	"strings"
	"sync"
	"time"
)

func handler(w http.ResponseWriter, r *http.Request) {
	start := time.Now()
	n := 1 << 20
	chunk := strings.Repeat("x", 1024)
	for i := 0; i < n; i++ {
		_, err := fmt.Fprintf(w, chunk)
		if err != nil {
			log.Printf("Error writing %v", err)
		}
	}

	size := float64(n) / 1024
	duration := time.Since(start).Seconds()
	log.Printf("Server: %f MBps", size/duration)
}

func main() {
	sv := httptest.NewUnstartedServer(http.HandlerFunc(handler))
	sv.EnableHTTP2 = true
	sv.StartTLS()

	var wg sync.WaitGroup

	tr, ok := sv.Client().Transport.(*http.Transport)
	if !ok {
		panic("transport unsupported")
	}
	tr.DisableKeepAlives = false

	workers := 4
	for i := 1; i <= workers; i++ {
		wg.Add(1)
		i := i
		go func() {
			defer wg.Done()
			start := time.Now()

			resp, err := sv.Client().Get(sv.URL + "/")
			if err != nil {
				log.Fatal(err)
			}
			defer resp.Body.Close()

			data, err := ioutil.ReadAll(resp.Body)
			if err != nil {
				log.Printf("Error reading %v", err)
			}

			size := float64(len(data)) / (1024 * 1024)
			duration := time.Since(start).Seconds()

			log.Printf("Client%d: %f MBps ", i, size/duration)
		}()
	}
	wg.Wait()

}

• tr.DisableKeepAlives = true

go run server.go 
2021/11/26 11:19:01 Server: 96.358534 MBps
2021/11/26 11:19:01 Client4: 96.338120 MBps 
2021/11/26 11:19:02 Server: 92.666090 MBps
2021/11/26 11:19:02 Client2: 92.651739 MBps 
2021/11/26 11:19:02 Server: 88.723179 MBps
2021/11/26 11:19:02 Client1: 88.693800 MBps 
2021/11/26 11:19:03 Server: 86.221247 MBps
2021/11/26 11:19:03 Client3: 86.193994 MBps 

• tr.DisableKeepAlives = false

$ go run server.go 
2021/11/26 11:19:27 Server: 63.471122 MBps
2021/11/26 11:19:27 Client4: 63.462420 MBps 
2021/11/26 11:19:29 Server: 58.067915 MBps
2021/11/26 11:19:29 Client3: 58.061386 MBps 
2021/11/26 11:19:35 Server: 43.437425 MBps
2021/11/26 11:19:35 Client1: 43.433545 MBps 
2021/11/26 11:19:38 Server: 38.357418 MBps
2021/11/26 11:19:38 Client2: 38.355121 MBps 

Setting tr.DisableKeepAlives = true creates one different connection per request, if I set to false I have half the throughput,

I hope this bug is not forgotten, it's a pretty important one...