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: ConfigureServer stops Shutdown from sending GOAWAY #18471

Closed
voutasaurus opened this issue Dec 30, 2016 · 8 comments
Closed

x/net/http2: ConfigureServer stops Shutdown from sending GOAWAY #18471

voutasaurus opened this issue Dec 30, 2016 · 8 comments
Assignees
Milestone

Comments

@voutasaurus
Copy link

@voutasaurus voutasaurus commented Dec 30, 2016

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

go version go1.8beta2 darwin/amd64

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

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/voutasaurus"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v_/gz6l428x5w97h0zrm5m03rlm0000gn/T/go-build157610117=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

Used this program and a self signed cert and key (generated by crypto/tls/generate_cert.go)

package main

import (
	"context"
	"flag"
	"fmt"
	"io"
	"log"
	"net/http"
	"time"

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

var flagConfigure = flag.Bool("configure", false, "call http2.ConfigureServer on server")

func main() {
	flag.Parse()
	unblock := make(chan bool, 1)
	h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		ctx := r.Context()

		println("REQUEST", ctx)

		for {
			select {
			case <-ctx.Done():
				println("DONE", ctx)
				return
			case unblock <- true:
				println("UNBLOCK", ctx)
			default:
			}

			println("WRITE", ctx)
			_, err := io.WriteString(w, ".")
			if err != nil {
				println("ERR", ctx, err.Error())
				return
			}

			w.(http.Flusher).Flush()

			println("SLEEP", ctx)
			time.Sleep(1 * time.Second)
		}
	})

	s := &http.Server{
		Addr:    ":8080",
		Handler: h,
	}

	if *flagConfigure {
		fmt.Println("CONFIGURING SERVER")
		http2.ConfigureServer(s, &http2.Server{})
	}

	go func() {
		<-unblock
		time.Sleep(5 * time.Second)
		ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
		defer cancel()
		println("SHUTDOWN")
		s.Shutdown(ctx)
	}()

	log.Println(s.ListenAndServeTLS("cert.pem", "key.pem"))
	select {}
}

Run this without -configure and it sends GOAWAY frames (seen in nghttp)

$ nghttp -nv https://localhost:8080                                                                                                                                                              [171/171]
[  0.002] Connected
The negotiated protocol: h2
[  0.003] recv SETTINGS frame <length=18, flags=0x00, stream_id=0>
          (niv=3)
          [SETTINGS_MAX_FRAME_SIZE(0x05):1048576]
          [SETTINGS_MAX_CONCURRENT_STREAMS(0x03):250]
          [SETTINGS_MAX_HEADER_LIST_SIZE(0x06):1048896]
[  0.003] send SETTINGS frame <length=12, flags=0x00, stream_id=0>
          (niv=2)
          [SETTINGS_MAX_CONCURRENT_STREAMS(0x03):100]
          [SETTINGS_INITIAL_WINDOW_SIZE(0x04):65535]
[  0.003] send SETTINGS frame <length=0, flags=0x01, stream_id=0>
          ; ACK
          (niv=0)
[  0.003] send PRIORITY frame <length=5, flags=0x00, stream_id=3>
          (dep_stream_id=0, weight=201, exclusive=0)
[  0.003] send PRIORITY frame <length=5, flags=0x00, stream_id=5>
          (dep_stream_id=0, weight=101, exclusive=0)
[  0.003] send PRIORITY frame <length=5, flags=0x00, stream_id=7>
          (dep_stream_id=0, weight=1, exclusive=0)
[  0.003] send PRIORITY frame <length=5, flags=0x00, stream_id=9>
          (dep_stream_id=7, weight=1, exclusive=0)
[  0.003] send PRIORITY frame <length=5, flags=0x00, stream_id=11>
          (dep_stream_id=3, weight=1, exclusive=0)
[  0.004] send HEADERS frame <length=38, flags=0x25, stream_id=13>
          ; END_STREAM | END_HEADERS | PRIORITY
          (padlen=0, dep_stream_id=11, weight=16, exclusive=0)
          ; Open new stream
          :method: GET
          :path: /
          :scheme: https
          :authority: localhost:8080
          accept: */*
          accept-encoding: gzip, deflate
          user-agent: nghttp2/1.13.0
[  0.004] recv SETTINGS frame <length=0, flags=0x01, stream_id=0>
          ; ACK
          (niv=0)
[  0.004] recv (stream_id=13) :status: 200
[  0.004] recv (stream_id=13) content-type: text/plain; charset=utf-8
[  0.004] recv (stream_id=13) date: Fri, 30 Dec 2016 01:14:53 GMT
[  0.004] recv HEADERS frame <length=45, flags=0x04, stream_id=13>
          ; END_HEADERS
          (padlen=0)
          ; First response header
[  0.004] recv DATA frame <length=1, flags=0x00, stream_id=13>
[  1.005] recv DATA frame <length=1, flags=0x00, stream_id=13>
[  2.007] recv DATA frame <length=1, flags=0x00, stream_id=13>
[  3.010] recv DATA frame <length=1, flags=0x00, stream_id=13>
[  4.012] recv DATA frame <length=1, flags=0x00, stream_id=13>
[  5.005] recv GOAWAY frame <length=8, flags=0x00, stream_id=0>
          (last_stream_id=13, error_code=NO_ERROR(0x00), opaque_data(0)=[])
[  5.015] recv DATA frame <length=1, flags=0x00, stream_id=13>
[  6.016] recv DATA frame <length=1, flags=0x00, stream_id=13>
[  7.019] recv DATA frame <length=1, flags=0x00, stream_id=13>

But with -configure, nghttp does not see a GOAWAY, only DATA frames.

What did you expect to see?

I expected http2.ConfigureServer to not stop Shutdown from sending GOAWAY frames on server connections.

What did you see instead?

http2.ConfigureServer stops Shutdown from sending GOAWAY frames.

[Note: Verified with latest golang.org/x/net/http2 SHA: 8fd7f25955530b92e73e9e1932a41b522b22ccd9]

@voutasaurus

This comment has been minimized.

Copy link
Author

@voutasaurus voutasaurus commented Dec 30, 2016

My guess here is that I shouldn't be using ConfigureServer at all but the behavior seems odd anyhow.

@voutasaurus

This comment has been minimized.

Copy link
Author

@voutasaurus voutasaurus commented Jan 1, 2017

In particular if you do something like this (https://github.com/golang/tools/blob/master/cmd/godoc/autocert.go#L38) then calling srv.Shutdown will not send GOAWAYs to clients.

@vcabbage

This comment has been minimized.

Copy link
Member

@vcabbage vcabbage commented Jan 2, 2017

I suspect the problem is that when you use http2.ConfigureServer, the server ends up referencing http2.h1ServerShutdownChan. As the comment says, that function only works when bundled into net/http, so your server can't get access to the shutdown channel.

// h1ServerShutdownChan returns a channel that will be closed when the
// provided *http.Server wants to shut down.
//
// This is a somewhat hacky way to get at http1 innards. It works
// when the http2 code is bundled into the net/http package in the
// standard library. The alternatives ended up making the cmd/go tool
// depend on http Servers. This is the lightest option for now.
// This is tested via the TestServeShutdown* tests in net/http.
func h1ServerShutdownChan(hs *http.Server) <-chan struct{} {
	if fn := testh1ServerShutdownChan; fn != nil {
		return fn(hs)
	}
	var x interface{} = hs
	type I interface {
		getDoneChan() <-chan struct{}
	}
	if hs, ok := x.(I); ok {
		return hs.getDoneChan()
	}
	return nil
}

https://github.com/golang/net/blob/8fd7f25955530b92e73e9e1932a41b522b22ccd9/http2/server.go#L2716-L2736

@voutasaurus

This comment has been minimized.

Copy link
Author

@voutasaurus voutasaurus commented Jan 2, 2017

That's correct, Kale.

I propose that calling the http2.ConfigureServer to configure max streams and such should not stop the bundled http.http2ConfigureServer from working. The latter is called in the callgraph of http.ListenAndServeTLS and should take precedence over the former for the purposes of GOAWAY.

I'm yet to figure out why http.http2ConfigureServer fails if http2.ConfigureServer is called before it.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 4, 2017

/cc @bradfitz

@rsc rsc added this to the Unreleased milestone Jan 4, 2017
@bradfitz bradfitz modified the milestones: Go1.9, Unreleased May 15, 2017
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 23, 2017

CL https://golang.org/cl/44003 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 23, 2017

CL https://golang.org/cl/43230 mentions this issue.

gopherbot pushed a commit that referenced this issue May 23, 2017
This will be used to allow http2 servers to register a shutdown function
so that net/http.Server.Shutdown will work when the http2 server is
configured via a manual call to http2.ConfigureServer. Currently, Shutdown
only works when the http2 server is configured automatically by the
net/http package.

Updates #20302
Updates #18471

Change-Id: Ifc2b5f3126126a106b49ea4a7e999279852b9cc9
Reviewed-on: https://go-review.googlesource.com/44003
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue May 24, 2017
This is a better fix that https://golang.org/cl/43455. Instead of
creating a separate goroutine to wait for the global shutdown channel,
we reuse the new serverMsgCh, which was added in a prior CL.

We also use the new net/http.Server.RegisterOnShutdown method to
register a shutdown callback for each http2.Server.

Updates golang/go#20302
Updates golang/go#18471

Change-Id: Icf29d5e4f65b3779d1fb4ea92924e4fb6bdadb2a
Reviewed-on: https://go-review.googlesource.com/43230
Run-TryBot: Tom Bergan <tombergan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 24, 2017

CL https://golang.org/cl/44006 mentions this issue.

@gopherbot gopherbot closed this in e6e6cad May 24, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
This is a better fix that https://golang.org/cl/43455. Instead of
creating a separate goroutine to wait for the global shutdown channel,
we reuse the new serverMsgCh, which was added in a prior CL.

We also use the new net/http.Server.RegisterOnShutdown method to
register a shutdown callback for each http2.Server.

Updates golang/go#20302
Updates golang/go#18471

Change-Id: Icf29d5e4f65b3779d1fb4ea92924e4fb6bdadb2a
Reviewed-on: https://go-review.googlesource.com/43230
Run-TryBot: Tom Bergan <tombergan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators May 24, 2018
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
6 participants
You can’t perform that action at this time.