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/h2c: http BaseContext/ConnContext methods are not used #37089

Open
jared2501 opened this issue Feb 6, 2020 · 14 comments
Open

x/net/http2/h2c: http BaseContext/ConnContext methods are not used #37089

jared2501 opened this issue Feb 6, 2020 · 14 comments
Labels
NeedsInvestigation
Milestone

Comments

@jared2501
Copy link

@jared2501 jared2501 commented Feb 6, 2020

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

go version go1.13.7 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What did you do? What did you expect to see? What did you see instead?

Currently, the http2/h2c package does not use the http.Server.BaseContext or http.Server.ConnContext methods. This means if I use the h2c package to upgrade a connection, the context for the user's is inherited from the background context rather than any context returned from the BaseContext or ConnContext functions.

A possible fix would be something like this:

diff --git a/http2/h2c/h2c.go b/http2/h2c/h2c.go
index 07c5c9a..349a5e6 100644
--- a/http2/h2c/h2c.go
+++ b/http2/h2c/h2c.go
@@ -11,6 +11,7 @@ package h2c
 import (
        "bufio"
        "bytes"
+       "context"
        "encoding/base64"
        "encoding/binary"
        "errors"
@@ -84,6 +85,10 @@ func (s h2cHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
                }
                defer conn.Close()

+               ctx := context.Background()
+               if server, ok := r.Context().Value(http.ServerContextKey).(*http.Server); ok && server.ConnContext != nil {
+                       ctx = server.ConnContext(ctx, conn)
+               }
                s.s.ServeConn(conn, &http2.ServeConnOpts{Handler: s.Handler})
                return
        }
@@ -91,6 +96,10 @@ func (s h2cHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
        if conn, err := h2cUpgrade(w, r); err == nil {
                defer conn.Close()

+               ctx := context.Background()
+               if server, ok := r.Context().Value(http.ServerContextKey).(*http.Server); ok && server.ConnContext != nil {
+                       ctx = server.ConnContext(ctx, conn)
+               }
                s.s.ServeConn(conn, &http2.ServeConnOpts{Handler: s.Handler})
                return
        }

Does this seem reasonable?

@dmitshur dmitshur changed the title net/http2/h2c: http BaseContext/ConnContext methods are not used x/net/http2/h2c: http BaseContext/ConnContext methods are not used Feb 7, 2020
@gopherbot gopherbot added this to the Unreleased milestone Feb 7, 2020
@dmitshur dmitshur added the NeedsInvestigation label Feb 7, 2020
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 7, 2020

/cc @bradfitz @tombergan per owners.

@networkimprov
Copy link

@networkimprov networkimprov commented Feb 7, 2020

cc @fraenkel re http2

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Feb 8, 2020

@jared2501 One question, now that you have created the context, what is going to use it?

@jared2501
Copy link
Author

@jared2501 jared2501 commented Feb 10, 2020

I was using it in downstream handler code so that I can track the number of HTTP/2 streams that exist per HTTP/2 connection. I.e. I have some code like this:

type connStreamCounter struct{}

var connStreamCounterKey = connStreamCounter{}

func (e *Endpoint) trackStreamsPerConn(ctx context.Context, add bool) {
	strmCtr := ctx.Value(connStreamCounterKey).(*atomicInt64)

	if add {
		numStreams := strmCtr.Add(1)
		...
		return
	}

	numStreams := strmCtr.Add(-1)
	...
}

@jared2501
Copy link
Author

@jared2501 jared2501 commented Feb 10, 2020

The reason for doing this is that we had an issue in production where a frontend web application was continually opening long-lived server streams, and never closing old streams. We then hit 250 open streams on a few of our HTTP/2 connections, which is the default max number before the http2.Server blocks accepting new streams. I would like to track this number so that I can monitor and alert on it.

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Feb 11, 2020

@jared2501 If you have set the MaxConcurrentStreams, the client side should be getting a stream error.
I know that there is a lack of desire to expose the net.Conn underneath the h2 or h2c implementations. I would have to defer to @bradfitz for an alternative way to track streams per connection.
You might be able to get away with retrieving the LocallAddrContextKey to manage the streams for a given connection in your own map.

@jared2501
Copy link
Author

@jared2501 jared2501 commented Feb 11, 2020

One thing to note that I would need the remote address, not just the local address, since the (local, remote) tuple fully identifies the connection. If I had this, then I could also achieve this goal so would also be happy with that solution.

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Feb 11, 2020

But you do have the remote address via the Request.

@jared2501
Copy link
Author

@jared2501 jared2501 commented Feb 18, 2020

@fraenkel ah you're totally right! So if I wanted to do this, I guess I could track this using a sync.Map.. It's not super ideal since A) I would have to do sync.Map read/writes for every request, and B) I wouldn't get notified when a connection ends so I wouldn't know when to clean up the counter.

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Feb 18, 2020

@jared2501 You could use srv.ConnState() to track that, no?

@jared2501
Copy link
Author

@jared2501 jared2501 commented Mar 16, 2020

@fraenkel The trouble is that once the HTTP/2 server takes over the ConnState will be StateHijacked, and so new HTTP/2 requests don't cause it to change.

@jared2501
Copy link
Author

@jared2501 jared2501 commented Mar 16, 2020

Ah I guess you're saying that after the connection ends, the ConnState will transition from StateHijacked to StateClosed?

@jared2501
Copy link
Author

@jared2501 jared2501 commented Mar 16, 2020

If so then yeah that's fine, we can close this ticket out

@cinchurge
Copy link

@cinchurge cinchurge commented Aug 27, 2020

we're implementing an auth system which uses ConnContext to extract some per-connection information that is used later for auth purposes:

  • when we receive a TCP connection, we do some expensive operations and store the result in the context to be used by all subsequent requests.
  • This is currently being done via ConnContext for HTTP1, and it's working great, but doesn't work when we use an h2c handler with HTTP/2.

we'd like to help make this change and have a preliminary patch ready. would that be something folks here would consider?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

6 participants