From 459513d1f8abff01b4854c93ff0bff7e87985a0a Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 27 Feb 2025 10:40:55 -0800 Subject: [PATCH] internal/http3: move more common stream processing to genericConn Move the server stream-accept loop into genericConn. (Overlooked in a previous CL.) Be more consistent about having genericConn handle errors. For golang/go#70914 Change-Id: I872673482f16539e95a1a1381ada7d3e22affb82 Reviewed-on: https://go-review.googlesource.com/c/net/+/653395 LUCI-TryBot-Result: Go LUCI Auto-Submit: Damien Neil Reviewed-by: Jonathan Amsterdam --- internal/http3/conn.go | 25 +++++++++++++++++++++---- internal/http3/server.go | 17 +++-------------- internal/http3/settings.go | 13 ++++++------- internal/http3/transport.go | 6 +++--- 4 files changed, 33 insertions(+), 28 deletions(-) diff --git a/internal/http3/conn.go b/internal/http3/conn.go index e9a58471e..5eb803115 100644 --- a/internal/http3/conn.go +++ b/internal/http3/conn.go @@ -19,7 +19,7 @@ type streamHandler interface { handlePushStream(*stream) error handleEncoderStream(*stream) error handleDecoderStream(*stream) error - handleRequestStream(*stream) + handleRequestStream(*stream) error abort(error) } @@ -43,7 +43,7 @@ func (c *genericConn) acceptStreams(qconn *quic.Conn, h streamHandler) { if st.IsReadOnly() { go c.handleUnidirectionalStream(newStream(st), h) } else { - go h.handleRequestStream(newStream(st)) + go c.handleRequestStream(newStream(st), h) } } } @@ -81,7 +81,6 @@ func (c *genericConn) handleUnidirectionalStream(st *stream, h streamHandler) { // but the quic package currently doesn't allow setting error codes // for STOP_SENDING frames. // TODO: Should CloseRead take an error code? - st.stream.CloseRead() err = nil } if err == io.EOF { @@ -90,8 +89,26 @@ func (c *genericConn) handleUnidirectionalStream(st *stream, h streamHandler) { message: streamType(stype).String() + " stream closed", } } - if err != nil { + c.handleStreamError(st, h, err) +} + +func (c *genericConn) handleRequestStream(st *stream, h streamHandler) { + c.handleStreamError(st, h, h.handleRequestStream(st)) +} + +func (c *genericConn) handleStreamError(st *stream, h streamHandler, err error) { + switch err := err.(type) { + case *connectionError: h.abort(err) + case nil: + st.stream.CloseRead() + st.stream.CloseWrite() + case *streamError: + st.stream.CloseRead() + st.stream.Reset(uint64(err.code)) + default: + st.stream.CloseRead() + st.stream.Reset(uint64(errH3InternalError)) } } diff --git a/internal/http3/server.go b/internal/http3/server.go index 2d8d1df22..ca93c5298 100644 --- a/internal/http3/server.go +++ b/internal/http3/server.go @@ -86,18 +86,7 @@ func newServerConn(qconn *quic.Conn) { controlStream.writeSettings() controlStream.Flush() - // Accept streams on the connection. - for { - st, err := sc.qconn.AcceptStream(context.Background()) - if err != nil { - return // connection closed - } - if st.IsReadOnly() { - go sc.handleUnidirectionalStream(newStream(st), sc) - } else { - go sc.handleRequestStream(newStream(st)) - } - } + sc.acceptStreams(sc.qconn, sc) } func (sc *serverConn) handleControlStream(st *stream) error { @@ -165,9 +154,9 @@ func (sc *serverConn) handlePushStream(*stream) error { } } -func (sc *serverConn) handleRequestStream(st *stream) { +func (sc *serverConn) handleRequestStream(st *stream) error { // TODO - return + return nil } // abort closes the connection with an error. diff --git a/internal/http3/settings.go b/internal/http3/settings.go index 45018aadd..b5e562eca 100644 --- a/internal/http3/settings.go +++ b/internal/http3/settings.go @@ -8,7 +8,6 @@ package http3 import ( "golang.org/x/net/internal/quic/quicwire" - "golang.org/x/net/quic" ) const ( @@ -39,9 +38,9 @@ func (st *stream) writeSettings(settings ...int64) { func (st *stream) readSettings(f func(settingType, value int64) error) error { frameType, err := st.readFrameHeader() if err != nil || frameType != frameTypeSettings { - return &quic.ApplicationError{ - Code: uint64(errH3MissingSettings), - Reason: "settings not sent on control stream", + return &connectionError{ + code: errH3MissingSettings, + message: "settings not sent on control stream", } } for st.lim > 0 { @@ -59,9 +58,9 @@ func (st *stream) readSettings(f func(settingType, value int64) error) error { // https://www.rfc-editor.org/rfc/rfc9114.html#section-7.2.4.1-5 switch settingsType { case 0x02, 0x03, 0x04, 0x05: - return &quic.ApplicationError{ - Code: uint64(errH3SettingsError), - Reason: "use of reserved setting", + return &connectionError{ + code: errH3SettingsError, + message: "use of reserved setting", } } diff --git a/internal/http3/transport.go b/internal/http3/transport.go index 83bc56c2b..b26524cbd 100644 --- a/internal/http3/transport.go +++ b/internal/http3/transport.go @@ -167,14 +167,14 @@ func (cc *ClientConn) handlePushStream(*stream) error { } } -func (cc *ClientConn) handleRequestStream(st *stream) { +func (cc *ClientConn) handleRequestStream(st *stream) error { // "Clients MUST treat receipt of a server-initiated bidirectional // stream as a connection error of type H3_STREAM_CREATION_ERROR [...]" // https://www.rfc-editor.org/rfc/rfc9114.html#section-6.1-3 - cc.abort(&connectionError{ + return &connectionError{ code: errH3StreamCreationError, message: "server created bidirectional stream", - }) + } } // abort closes the connection with an error.