From d3892a56df4ed843e828a07e02df4cede2f0d804 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 1 Oct 2021 16:17:34 -0700 Subject: [PATCH 1/5] server: add missing conn.Close if the connection dies before reading the HTTP/2 preface --- internal/transport/http2_server.go | 2 +- test/end2end_test.go | 58 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/internal/transport/http2_server.go b/internal/transport/http2_server.go index 3e4655e8e65..09c6fad4b1e 100644 --- a/internal/transport/http2_server.go +++ b/internal/transport/http2_server.go @@ -293,7 +293,7 @@ func NewServerTransport(conn net.Conn, config *ServerConfig) (_ ServerTransport, // closed immediately by the latter. Skipping the error here will help // reduce log clutter. if err == io.EOF { - return nil, nil + return nil, io.EOF } return nil, connectionErrorf(false, err, "transport: http2Server.HandleStreams failed to receive the preface from client: %v", err) } diff --git a/test/end2end_test.go b/test/end2end_test.go index 9d7fcb23206..e3a78b24cc0 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -7990,3 +7990,61 @@ func (s) TestAuthorityHeader(t *testing.T) { }) } } + +// wrapCloseListener tracks Accepts/Closes and maintains a counter of the +// number of open connections. +type wrapCloseListener struct { + net.Listener + connsOpen int32 +} + +// wrapCloseListener is returned by wrapCloseListener.Accept and decrements its +// connsOpen when Close is called. +type wrapCloseConn struct { + net.Conn + lis *wrapCloseListener + closeOnce sync.Once +} + +func (w *wrapCloseListener) Accept() (net.Conn, error) { + conn, err := w.Listener.Accept() + if err != nil { + return nil, err + } + atomic.AddInt32(&w.connsOpen, 1) + return &wrapCloseConn{Conn: conn, lis: w}, nil +} + +func (w *wrapCloseConn) Close() error { + defer w.closeOnce.Do(func() { atomic.AddInt32(&w.lis.connsOpen, -1) }) + return w.Conn.Close() +} + +// TestServerClosesConn ensures conn.Close is always closed even if the client +// doesn't complete the HTTP/2 handshake. +func (s) TestServerClosesConn(t *testing.T) { + lis := bufconn.Listen(20) + wrapLis := &wrapCloseListener{Listener: lis} + + s := grpc.NewServer() + go s.Serve(wrapLis) + defer s.Stop() + + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + for i := 0; i < 10; i++ { + conn, err := lis.DialContext(ctx) + if err != nil { + t.Fatalf("Dial = _, %v; want _, nil", err) + } + conn.Close() + } + for ctx.Err() == nil { + time.Sleep(50 * time.Millisecond) + if atomic.LoadInt32(&wrapLis.connsOpen) == 0 { + return + } + } + t.Fatalf("timed out waiting for conns to be closed by server; still open: %v", atomic.LoadInt32(&wrapLis.connsOpen)) +} From 3a34ae085ef83b1978d88bff5a2fc387053b77ac Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 1 Oct 2021 16:21:41 -0700 Subject: [PATCH 2/5] minor cleanup --- server.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server.go b/server.go index 1e82c955529..0474e46f483 100644 --- a/server.go +++ b/server.go @@ -888,10 +888,8 @@ func (s *Server) newHTTP2Transport(c net.Conn) transport.ServerTransport { c.Close() } // Don't log on ErrConnDispatched and io.EOF to prevent log spam. - if err != credentials.ErrConnDispatched { - if err != io.EOF { - channelz.Warning(logger, s.channelzID, "grpc: Server.Serve failed to create ServerTransport: ", err) - } + if err != credentials.ErrConnDispatched && err != io.EOF { + channelz.Warning(logger, s.channelzID, "grpc: Server.Serve failed to create ServerTransport: ", err) } return nil } From c4ee7e00ff2f196933a7433d242ce1564942a6c0 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 1 Oct 2021 16:22:14 -0700 Subject: [PATCH 3/5] minor cleanup 2 --- server.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server.go b/server.go index 0474e46f483..eadf9e05fd1 100644 --- a/server.go +++ b/server.go @@ -885,12 +885,12 @@ func (s *Server) newHTTP2Transport(c net.Conn) transport.ServerTransport { // ErrConnDispatched means that the connection was dispatched away from // gRPC; those connections should be left open. if err != credentials.ErrConnDispatched { + // Don't log on ErrConnDispatched and io.EOF to prevent log spam. + if err != io.EOF { + channelz.Warning(logger, s.channelzID, "grpc: Server.Serve failed to create ServerTransport: ", err) + } c.Close() } - // Don't log on ErrConnDispatched and io.EOF to prevent log spam. - if err != credentials.ErrConnDispatched && err != io.EOF { - channelz.Warning(logger, s.channelzID, "grpc: Server.Serve failed to create ServerTransport: ", err) - } return nil } From 48c8294ab3617321f137edc1d35dee5477dc1c27 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 1 Oct 2021 16:24:25 -0700 Subject: [PATCH 4/5] be more optimistic --- test/end2end_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/end2end_test.go b/test/end2end_test.go index e3a78b24cc0..957d13f731f 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -8041,10 +8041,10 @@ func (s) TestServerClosesConn(t *testing.T) { conn.Close() } for ctx.Err() == nil { - time.Sleep(50 * time.Millisecond) if atomic.LoadInt32(&wrapLis.connsOpen) == 0 { return } + time.Sleep(50 * time.Millisecond) } t.Fatalf("timed out waiting for conns to be closed by server; still open: %v", atomic.LoadInt32(&wrapLis.connsOpen)) } From 709d464189ac3cced4a52332bbf147e2beeebf61 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Mon, 4 Oct 2021 09:42:00 -0700 Subject: [PATCH 5/5] comment tweak --- internal/transport/http2_server.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/transport/http2_server.go b/internal/transport/http2_server.go index 09c6fad4b1e..f2cad9ebc31 100644 --- a/internal/transport/http2_server.go +++ b/internal/transport/http2_server.go @@ -290,8 +290,9 @@ func NewServerTransport(conn net.Conn, config *ServerConfig) (_ ServerTransport, if _, err := io.ReadFull(t.conn, preface); err != nil { // In deployments where a gRPC server runs behind a cloud load balancer // which performs regular TCP level health checks, the connection is - // closed immediately by the latter. Skipping the error here will help - // reduce log clutter. + // closed immediately by the latter. Returning io.EOF here allows the + // grpc server implementation to recognize this scenario and suppress + // logging to reduce spam. if err == io.EOF { return nil, io.EOF }