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

x/net/http2: do not flush immediately after write headers #58674

Open
veezhang opened this issue Feb 24, 2023 · 7 comments
Open

x/net/http2: do not flush immediately after write headers #58674

veezhang opened this issue Feb 24, 2023 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@veezhang
Copy link
Contributor

The http2 client flush immediately after write headers, so there have two TCP packets separated.

https://github.com/golang/net/blob/master/http2/transport.go#L1617

When a large number of requests, this may cause a performance loss.

So should combine the header and body packets, and do not flush immediately after write headers?

@gopherbot gopherbot added this to the Proposal milestone Feb 24, 2023
@veezhang
Copy link
Contributor Author

veezhang commented Feb 24, 2023

I create an discussion, and there seems to be no professional answer.

@ianlancetaylor
Copy link
Contributor

This doesn't seem like an API change, so taking it out of the proposal process.

CC @neild @bradfitz

@ianlancetaylor ianlancetaylor changed the title proposal: x/net/http2: do not flush immediately after write headers x/net/http2: do not flush immediately after write headers Feb 24, 2023
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Feb 24, 2023
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unreleased Feb 24, 2023
@neild
Copy link
Contributor

neild commented Feb 24, 2023

Not sure why that flush is there.

A concern in trying to change this is the case of a request that doesn't send the body until it reads some portion of the response. Not the common case, but something allowed when treating an HTTP/2 request as a bidirectional stream. Just dropping this flush would mean the request never gets sent.

I'd want a solid answer for how that case works, or why it doesn't matter, before trying to change anything here.

@veezhang
Copy link
Contributor Author

veezhang commented Feb 27, 2023

@neild Hi, I make a test for it.

The server code:

package main

import (
	"net/http"

	"golang.org/x/net/http2"
	"golang.org/x/net/http2/h2c"
)

func main() {
	h2s := &http2.Server{}
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

	})
	server := &http.Server{
		Addr:    "0.0.0.0:28080",
		Handler: h2c.NewHandler(handler, h2s),
	}
	if err := server.ListenAndServe(); err != nil {
		panic(err)
	}
}

The client code:

package main

import (
	"context"
	"crypto/tls"
	"fmt"
	"net"
	"net/http"
	"strings"
	"sync"
	"time"

	"golang.org/x/net/http2"
)

func main() {
	client := http.Client{
		Transport: &http2.Transport{
			AllowHTTP: true,
			DialTLSContext: func(_ context.Context, network, addr string, _ *tls.Config) (net.Conn, error) {
				return net.Dial(network, addr)
			},
		},
	}

	var (
		startTime   = time.Now()
		wg          sync.WaitGroup
		concurrency = 10
		count       = 100000
	)
	for i := 0; i < concurrency; i++ {
		wg.Add(1)
		go func() {
			worker(client, count)
			wg.Done()
		}()
	}
	wg.Wait()
	dur := time.Since(startTime)
	fmt.Printf("%s %d %.2f\n", dur, 10*100000, float64(10*100000)/dur.Seconds())
}

func worker(client http.Client, n int) {
	for i := 0; i < n; i++ {
		_, err := client.Post(
			"http://192.168.0.4:28080",
			"application/json",
			strings.NewReader(`{"key":"value"}`),
		)
		if err != nil {
			panic(err)
		}
	}
}

When I use the original golang.org/x/net/http2 package, the test looks like this:

➜  go run ./test-h2c/client/client.go
42.616582576s 1000000 23465.04
➜  go run ./test-h2c/client/client.go
43.251295758s 1000000 23120.69

After I comment out cc.bw.Flush()

diff --git a/http2/transport.go b/http2/transport.go
index 05ba23d..a09df11 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -1614,7 +1614,7 @@ func (cc *ClientConn) writeHeaders(streamID uint32, endStream bool, maxFrameSize
                        cc.fr.WriteContinuation(streamID, endHeaders, chunk)
                }
        }
-       cc.bw.Flush()
+       // cc.bw.Flush()
        return cc.werr
 }

And test again, as follows:

➜  go run ./test-h2c/client/client.go
29.949843356s 1000000 33389.16
➜  go run ./test-h2c/client/client.go
29.071164913s 1000000 34398.35

Judging from the test situation, there is an improvement of more than 40%.
Moreover, it can also be seen through tcpdump capture that there are 2 packets before the modification, and they are merged into one after the modification.

image

image

@neild
Copy link
Contributor

neild commented Mar 4, 2023

I agree that this change decreases POST latency. That's not in question.

My concern is that it's a change in user-visible behavior: A POST request is now not sent until the first read from the body returns. I don't know if that breaks any existing users. If someone does want to read from the response body before sending the first request body byte, how do they do that after this change?

@veezhang
Copy link
Contributor Author

@neild I don't think it's necessary to send headers separately. Of course, you can evaluate whether there is a problem with waiting to read from the body before sending the header?

@veezhang
Copy link
Contributor Author

veezhang commented Mar 31, 2023

Also, if there is a problem. So is it possible to add a waiting timeout for sending the header?
It's should support those cases:

  • sending immediately, this is the default for compatibility.
  • waiting for a timeout
  • waiting until the body read return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants