From 5a444b4f2fe893ea00f0376da46aa5376c3f3e28 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 29 Nov 2023 10:41:41 -0800 Subject: [PATCH 01/22] quic: add Stream.Set{Read,Write}Context, drop {Read,Write,Close}Context The ReadContext, WriteContext, and CloseContext Stream methods are difficult to use in conjunction with functions that operate on an io.Reader, io.Writer, or io.Closer. For example, it's reasonable to want to use io.ReadFull with a Stream, but doing so with a context is cumbersome. Drop the Stream methods that take a Context in favor of stateful methods that set the Context to use for read and write operations. (Close counts as a write operation, since it blocks waiting for data to be sent.) Intentionally make Set{Read,Write}Context not concurrency safe, to allow the race detector to catch misuse. This shouldn't be a problem for correct programs, since reads and writes are inherently not concurrency-safe. For golang/go#58547 Change-Id: I41378eb552d89a720921fc8644d3637c1a545676 Reviewed-on: https://go-review.googlesource.com/c/net/+/550795 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- internal/quic/conn_close_test.go | 11 ++- internal/quic/conn_flow_test.go | 19 ++-- internal/quic/conn_loss_test.go | 9 +- internal/quic/conn_streams_test.go | 26 +++--- internal/quic/conn_test.go | 11 +++ internal/quic/stream.go | 73 ++++++++------- internal/quic/stream_limits_test.go | 9 +- internal/quic/stream_test.go | 139 ++++++++++++++-------------- 8 files changed, 154 insertions(+), 143 deletions(-) diff --git a/internal/quic/conn_close_test.go b/internal/quic/conn_close_test.go index 63d4911e8..213975011 100644 --- a/internal/quic/conn_close_test.go +++ b/internal/quic/conn_close_test.go @@ -249,8 +249,9 @@ func TestConnCloseUnblocksNewStream(t *testing.T) { func TestConnCloseUnblocksStreamRead(t *testing.T) { testConnCloseUnblocks(t, func(ctx context.Context, tc *testConn) error { s := newLocalStream(t, tc, bidiStream) + s.SetReadContext(ctx) buf := make([]byte, 16) - _, err := s.ReadContext(ctx, buf) + _, err := s.Read(buf) return err }, permissiveTransportParameters) } @@ -258,8 +259,9 @@ func TestConnCloseUnblocksStreamRead(t *testing.T) { func TestConnCloseUnblocksStreamWrite(t *testing.T) { testConnCloseUnblocks(t, func(ctx context.Context, tc *testConn) error { s := newLocalStream(t, tc, bidiStream) + s.SetWriteContext(ctx) buf := make([]byte, 32) - _, err := s.WriteContext(ctx, buf) + _, err := s.Write(buf) return err }, permissiveTransportParameters, func(c *Config) { c.MaxStreamWriteBufferSize = 16 @@ -269,11 +271,12 @@ func TestConnCloseUnblocksStreamWrite(t *testing.T) { func TestConnCloseUnblocksStreamClose(t *testing.T) { testConnCloseUnblocks(t, func(ctx context.Context, tc *testConn) error { s := newLocalStream(t, tc, bidiStream) + s.SetWriteContext(ctx) buf := make([]byte, 16) - _, err := s.WriteContext(ctx, buf) + _, err := s.Write(buf) if err != nil { return err } - return s.CloseContext(ctx) + return s.Close() }, permissiveTransportParameters) } diff --git a/internal/quic/conn_flow_test.go b/internal/quic/conn_flow_test.go index 39c879346..8e04e20d9 100644 --- a/internal/quic/conn_flow_test.go +++ b/internal/quic/conn_flow_test.go @@ -12,7 +12,6 @@ import ( ) func TestConnInflowReturnOnRead(t *testing.T) { - ctx := canceledContext() tc, s := newTestConnAndRemoteStream(t, serverSide, uniStream, func(c *Config) { c.MaxConnReadBufferSize = 64 }) @@ -21,14 +20,14 @@ func TestConnInflowReturnOnRead(t *testing.T) { data: make([]byte, 64), }) const readSize = 8 - if n, err := s.ReadContext(ctx, make([]byte, readSize)); n != readSize || err != nil { + if n, err := s.Read(make([]byte, readSize)); n != readSize || err != nil { t.Fatalf("s.Read() = %v, %v; want %v, nil", n, err, readSize) } tc.wantFrame("available window increases, send a MAX_DATA", packetType1RTT, debugFrameMaxData{ max: 64 + readSize, }) - if n, err := s.ReadContext(ctx, make([]byte, 64)); n != 64-readSize || err != nil { + if n, err := s.Read(make([]byte, 64)); n != 64-readSize || err != nil { t.Fatalf("s.Read() = %v, %v; want %v, nil", n, err, 64-readSize) } tc.wantFrame("available window increases, send a MAX_DATA", @@ -42,7 +41,7 @@ func TestConnInflowReturnOnRead(t *testing.T) { data: make([]byte, 64), }) tc.wantIdle("connection is idle") - if n, err := s.ReadContext(ctx, make([]byte, 64)); n != 64 || err != nil { + if n, err := s.Read(make([]byte, 64)); n != 64 || err != nil { t.Fatalf("offset 64: s.Read() = %v, %v; want %v, nil", n, err, 64) } } @@ -79,10 +78,10 @@ func TestConnInflowReturnOnRacingReads(t *testing.T) { t.Fatalf("conn.AcceptStream() = %v", err) } read1 := runAsync(tc, func(ctx context.Context) (int, error) { - return s1.ReadContext(ctx, make([]byte, 16)) + return s1.Read(make([]byte, 16)) }) read2 := runAsync(tc, func(ctx context.Context) (int, error) { - return s2.ReadContext(ctx, make([]byte, 1)) + return s2.Read(make([]byte, 1)) }) // This MAX_DATA might extend the window by 16 or 17, depending on // whether the second write occurs before the update happens. @@ -90,10 +89,10 @@ func TestConnInflowReturnOnRacingReads(t *testing.T) { packetType1RTT, debugFrameMaxData{}) tc.wantIdle("redundant MAX_DATA is not sent") if _, err := read1.result(); err != nil { - t.Errorf("ReadContext #1 = %v", err) + t.Errorf("Read #1 = %v", err) } if _, err := read2.result(); err != nil { - t.Errorf("ReadContext #2 = %v", err) + t.Errorf("Read #2 = %v", err) } } @@ -227,13 +226,13 @@ func TestConnInflowMultipleStreams(t *testing.T) { t.Fatalf("AcceptStream() = %v", err) } streams = append(streams, s) - if n, err := s.ReadContext(ctx, make([]byte, 1)); err != nil || n != 1 { + if n, err := s.Read(make([]byte, 1)); err != nil || n != 1 { t.Fatalf("s.Read() = %v, %v; want 1, nil", n, err) } } tc.wantIdle("streams have read data, but not enough to update MAX_DATA") - if n, err := streams[0].ReadContext(ctx, make([]byte, 32)); err != nil || n != 31 { + if n, err := streams[0].Read(make([]byte, 32)); err != nil || n != 31 { t.Fatalf("s.Read() = %v, %v; want 31, nil", n, err) } tc.wantFrame("read enough data to trigger a MAX_DATA update", diff --git a/internal/quic/conn_loss_test.go b/internal/quic/conn_loss_test.go index 818816335..876ffd093 100644 --- a/internal/quic/conn_loss_test.go +++ b/internal/quic/conn_loss_test.go @@ -433,7 +433,8 @@ func TestLostMaxStreamsFrameMostRecent(t *testing.T) { if err != nil { t.Fatalf("AcceptStream() = %v", err) } - s.CloseContext(ctx) + s.SetWriteContext(ctx) + s.Close() if styp == bidiStream { tc.wantFrame("stream is closed", packetType1RTT, debugFrameStream{ @@ -480,7 +481,7 @@ func TestLostMaxStreamsFrameNotMostRecent(t *testing.T) { if err != nil { t.Fatalf("AcceptStream() = %v", err) } - if err := s.CloseContext(ctx); err != nil { + if err := s.Close(); err != nil { t.Fatalf("stream.Close() = %v", err) } tc.wantFrame("closing stream updates peer's MAX_STREAMS", @@ -512,7 +513,7 @@ func TestLostStreamDataBlockedFrame(t *testing.T) { }) w := runAsync(tc, func(ctx context.Context) (int, error) { - return s.WriteContext(ctx, []byte{0, 1, 2, 3}) + return s.Write([]byte{0, 1, 2, 3}) }) defer w.cancel() tc.wantFrame("write is blocked by flow control", @@ -564,7 +565,7 @@ func TestLostStreamDataBlockedFrameAfterStreamUnblocked(t *testing.T) { data := []byte{0, 1, 2, 3} w := runAsync(tc, func(ctx context.Context) (int, error) { - return s.WriteContext(ctx, data) + return s.Write(data) }) defer w.cancel() tc.wantFrame("write is blocked by flow control", diff --git a/internal/quic/conn_streams_test.go b/internal/quic/conn_streams_test.go index 6815e403e..dc81ad991 100644 --- a/internal/quic/conn_streams_test.go +++ b/internal/quic/conn_streams_test.go @@ -230,8 +230,8 @@ func TestStreamsWriteQueueFairness(t *testing.T) { t.Fatal(err) } streams = append(streams, s) - if n, err := s.WriteContext(ctx, data); n != len(data) || err != nil { - t.Fatalf("s.WriteContext() = %v, %v; want %v, nil", n, err, len(data)) + if n, err := s.Write(data); n != len(data) || err != nil { + t.Fatalf("s.Write() = %v, %v; want %v, nil", n, err, len(data)) } // Wait for the stream to finish writing whatever frames it can before // congestion control blocks it. @@ -298,7 +298,7 @@ func TestStreamsShutdown(t *testing.T) { side: localStream, styp: uniStream, setup: func(t *testing.T, tc *testConn, s *Stream) { - s.CloseContext(canceledContext()) + s.Close() }, shutdown: func(t *testing.T, tc *testConn, s *Stream) { tc.writeAckForAll() @@ -311,7 +311,7 @@ func TestStreamsShutdown(t *testing.T) { tc.writeFrames(packetType1RTT, debugFrameResetStream{ id: s.id, }) - s.CloseContext(canceledContext()) + s.Close() }, shutdown: func(t *testing.T, tc *testConn, s *Stream) { tc.writeAckForAll() @@ -321,8 +321,8 @@ func TestStreamsShutdown(t *testing.T) { side: localStream, styp: bidiStream, setup: func(t *testing.T, tc *testConn, s *Stream) { - s.CloseContext(canceledContext()) - tc.wantIdle("all frames after CloseContext are ignored") + s.Close() + tc.wantIdle("all frames after Close are ignored") tc.writeAckForAll() }, shutdown: func(t *testing.T, tc *testConn, s *Stream) { @@ -335,13 +335,12 @@ func TestStreamsShutdown(t *testing.T) { side: remoteStream, styp: uniStream, setup: func(t *testing.T, tc *testConn, s *Stream) { - ctx := canceledContext() tc.writeFrames(packetType1RTT, debugFrameStream{ id: s.id, fin: true, }) - if n, err := s.ReadContext(ctx, make([]byte, 16)); n != 0 || err != io.EOF { - t.Errorf("ReadContext() = %v, %v; want 0, io.EOF", n, err) + if n, err := s.Read(make([]byte, 16)); n != 0 || err != io.EOF { + t.Errorf("Read() = %v, %v; want 0, io.EOF", n, err) } }, shutdown: func(t *testing.T, tc *testConn, s *Stream) { @@ -451,17 +450,14 @@ func TestStreamsCreateAndCloseRemote(t *testing.T) { id: op.id, }) case acceptOp: - s, err := tc.conn.AcceptStream(ctx) - if err != nil { - t.Fatalf("AcceptStream() = %q; want stream %v", err, stringID(op.id)) - } + s := tc.acceptStream() if s.id != op.id { - t.Fatalf("accepted stram %v; want stream %v", err, stringID(op.id)) + t.Fatalf("accepted stream %v; want stream %v", stringID(s.id), stringID(op.id)) } t.Logf("accepted stream %v", stringID(op.id)) // Immediately close the stream, so the stream becomes done when the // peer closes its end. - s.CloseContext(ctx) + s.Close() } p := tc.readPacket() if p != nil { diff --git a/internal/quic/conn_test.go b/internal/quic/conn_test.go index ddf0740e2..2d3c946d6 100644 --- a/internal/quic/conn_test.go +++ b/internal/quic/conn_test.go @@ -382,6 +382,17 @@ func (tc *testConn) cleanup() { <-tc.conn.donec } +func (tc *testConn) acceptStream() *Stream { + tc.t.Helper() + s, err := tc.conn.AcceptStream(canceledContext()) + if err != nil { + tc.t.Fatalf("conn.AcceptStream() = %v, want stream", err) + } + s.SetReadContext(canceledContext()) + s.SetWriteContext(canceledContext()) + return s +} + func logDatagram(t *testing.T, text string, d *testDatagram) { t.Helper() if !*testVV { diff --git a/internal/quic/stream.go b/internal/quic/stream.go index fb9c1cf3c..d0122b951 100644 --- a/internal/quic/stream.go +++ b/internal/quic/stream.go @@ -18,6 +18,11 @@ type Stream struct { id streamID conn *Conn + // Contexts used for read/write operations. + // Intentionally not mutex-guarded, to allow the race detector to catch concurrent access. + inctx context.Context + outctx context.Context + // ingate's lock guards all receive-related state. // // The gate condition is set if a read from the stream will not block, @@ -152,6 +157,8 @@ func newStream(c *Conn, id streamID) *Stream { inresetcode: -1, // -1 indicates no RESET_STREAM received ingate: newLockedGate(), outgate: newLockedGate(), + inctx: context.Background(), + outctx: context.Background(), } if !s.IsReadOnly() { s.outdone = make(chan struct{}) @@ -159,6 +166,22 @@ func newStream(c *Conn, id streamID) *Stream { return s } +// SetReadContext sets the context used for reads from the stream. +// +// It is not safe to call SetReadContext concurrently. +func (s *Stream) SetReadContext(ctx context.Context) { + s.inctx = ctx +} + +// SetWriteContext sets the context used for writes to the stream. +// The write context is also used by Close when waiting for writes to be +// received by the peer. +// +// It is not safe to call SetWriteContext concurrently. +func (s *Stream) SetWriteContext(ctx context.Context) { + s.outctx = ctx +} + // IsReadOnly reports whether the stream is read-only // (a unidirectional stream created by the peer). func (s *Stream) IsReadOnly() bool { @@ -172,24 +195,18 @@ func (s *Stream) IsWriteOnly() bool { } // Read reads data from the stream. -// See ReadContext for more details. -func (s *Stream) Read(b []byte) (n int, err error) { - return s.ReadContext(context.Background(), b) -} - -// ReadContext reads data from the stream. // -// ReadContext returns as soon as at least one byte of data is available. +// Read returns as soon as at least one byte of data is available. // -// If the peer closes the stream cleanly, ReadContext returns io.EOF after +// If the peer closes the stream cleanly, Read returns io.EOF after // returning all data sent by the peer. -// If the peer aborts reads on the stream, ReadContext returns +// If the peer aborts reads on the stream, Read returns // an error wrapping StreamResetCode. -func (s *Stream) ReadContext(ctx context.Context, b []byte) (n int, err error) { +func (s *Stream) Read(b []byte) (n int, err error) { if s.IsWriteOnly() { return 0, errors.New("read from write-only stream") } - if err := s.ingate.waitAndLock(ctx, s.conn.testHooks); err != nil { + if err := s.ingate.waitAndLock(s.inctx, s.conn.testHooks); err != nil { return 0, err } defer func() { @@ -237,17 +254,11 @@ func shouldUpdateFlowControl(maxWindow, addedWindow int64) bool { } // Write writes data to the stream. -// See WriteContext for more details. -func (s *Stream) Write(b []byte) (n int, err error) { - return s.WriteContext(context.Background(), b) -} - -// WriteContext writes data to the stream. // -// WriteContext writes data to the stream write buffer. +// Write writes data to the stream write buffer. // Buffered data is only sent when the buffer is sufficiently full. // Call the Flush method to ensure buffered data is sent. -func (s *Stream) WriteContext(ctx context.Context, b []byte) (n int, err error) { +func (s *Stream) Write(b []byte) (n int, err error) { if s.IsReadOnly() { return 0, errors.New("write to read-only stream") } @@ -259,7 +270,7 @@ func (s *Stream) WriteContext(ctx context.Context, b []byte) (n int, err error) if len(b) > 0 && !canWrite { // Our send buffer is full. Wait for the peer to ack some data. s.outUnlock() - if err := s.outgate.waitAndLock(ctx, s.conn.testHooks); err != nil { + if err := s.outgate.waitAndLock(s.outctx, s.conn.testHooks); err != nil { return n, err } // Successfully returning from waitAndLockGate means we are no longer @@ -317,7 +328,7 @@ func (s *Stream) WriteContext(ctx context.Context, b []byte) (n int, err error) // Flush flushes data written to the stream. // It does not wait for the peer to acknowledge receipt of the data. -// Use CloseContext to wait for the peer's acknowledgement. +// Use Close to wait for the peer's acknowledgement. func (s *Stream) Flush() { s.outgate.lock() defer s.outUnlock() @@ -333,27 +344,21 @@ func (s *Stream) flushLocked() { } // Close closes the stream. -// See CloseContext for more details. -func (s *Stream) Close() error { - return s.CloseContext(context.Background()) -} - -// CloseContext closes the stream. // Any blocked stream operations will be unblocked and return errors. // -// CloseContext flushes any data in the stream write buffer and waits for the peer to +// Close flushes any data in the stream write buffer and waits for the peer to // acknowledge receipt of the data. // If the stream has been reset, it waits for the peer to acknowledge the reset. // If the context expires before the peer receives the stream's data, -// CloseContext discards the buffer and returns the context error. -func (s *Stream) CloseContext(ctx context.Context) error { +// Close discards the buffer and returns the context error. +func (s *Stream) Close() error { s.CloseRead() if s.IsReadOnly() { return nil } s.CloseWrite() // TODO: Return code from peer's RESET_STREAM frame? - if err := s.conn.waitOnDone(ctx, s.outdone); err != nil { + if err := s.conn.waitOnDone(s.outctx, s.outdone); err != nil { return err } s.outgate.lock() @@ -369,7 +374,7 @@ func (s *Stream) CloseContext(ctx context.Context) error { // // CloseRead notifies the peer that the stream has been closed for reading. // It does not wait for the peer to acknowledge the closure. -// Use CloseContext to wait for the peer's acknowledgement. +// Use Close to wait for the peer's acknowledgement. func (s *Stream) CloseRead() { if s.IsWriteOnly() { return @@ -394,7 +399,7 @@ func (s *Stream) CloseRead() { // // CloseWrite sends any data in the stream write buffer to the peer. // It does not wait for the peer to acknowledge receipt of the data. -// Use CloseContext to wait for the peer's acknowledgement. +// Use Close to wait for the peer's acknowledgement. func (s *Stream) CloseWrite() { if s.IsReadOnly() { return @@ -412,7 +417,7 @@ func (s *Stream) CloseWrite() { // Reset sends the application protocol error code, which must be // less than 2^62, to the peer. // It does not wait for the peer to acknowledge receipt of the error. -// Use CloseContext to wait for the peer's acknowledgement. +// Use Close to wait for the peer's acknowledgement. // // Reset does not affect reads. // Use CloseRead to abort reads on the stream. diff --git a/internal/quic/stream_limits_test.go b/internal/quic/stream_limits_test.go index 3f291e9f4..9c2f71ec1 100644 --- a/internal/quic/stream_limits_test.go +++ b/internal/quic/stream_limits_test.go @@ -200,7 +200,6 @@ func TestStreamLimitMaxStreamsFrameTooLarge(t *testing.T) { func TestStreamLimitSendUpdatesMaxStreams(t *testing.T) { testStreamTypes(t, "", func(t *testing.T, styp streamType) { - ctx := canceledContext() tc := newTestConn(t, serverSide, func(c *Config) { if styp == uniStream { c.MaxUniRemoteStreams = 4 @@ -218,13 +217,9 @@ func TestStreamLimitSendUpdatesMaxStreams(t *testing.T) { id: newStreamID(clientSide, styp, int64(i)), fin: true, }) - s, err := tc.conn.AcceptStream(ctx) - if err != nil { - t.Fatalf("AcceptStream = %v", err) - } - streams = append(streams, s) + streams = append(streams, tc.acceptStream()) } - streams[3].CloseContext(ctx) + streams[3].Close() if styp == bidiStream { tc.wantFrame("stream is closed", packetType1RTT, debugFrameStream{ diff --git a/internal/quic/stream_test.go b/internal/quic/stream_test.go index 00e392dba..08e89b24c 100644 --- a/internal/quic/stream_test.go +++ b/internal/quic/stream_test.go @@ -19,7 +19,6 @@ import ( func TestStreamWriteBlockedByOutputBuffer(t *testing.T) { testStreamTypes(t, "", func(t *testing.T, styp streamType) { - ctx := canceledContext() want := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} const writeBufferSize = 4 tc := newTestConn(t, clientSide, permissiveTransportParameters, func(c *Config) { @@ -28,15 +27,12 @@ func TestStreamWriteBlockedByOutputBuffer(t *testing.T) { tc.handshake() tc.ignoreFrame(frameTypeAck) - s, err := tc.conn.newLocalStream(ctx, styp) - if err != nil { - t.Fatal(err) - } + s := newLocalStream(t, tc, styp) // Non-blocking write. - n, err := s.WriteContext(ctx, want) + n, err := s.Write(want) if n != writeBufferSize || err != context.Canceled { - t.Fatalf("s.WriteContext() = %v, %v; want %v, context.Canceled", n, err, writeBufferSize) + t.Fatalf("s.Write() = %v, %v; want %v, context.Canceled", n, err, writeBufferSize) } s.Flush() tc.wantFrame("first write buffer of data sent", @@ -48,7 +44,8 @@ func TestStreamWriteBlockedByOutputBuffer(t *testing.T) { // Blocking write, which must wait for buffer space. w := runAsync(tc, func(ctx context.Context) (int, error) { - n, err := s.WriteContext(ctx, want[writeBufferSize:]) + s.SetWriteContext(ctx) + n, err := s.Write(want[writeBufferSize:]) s.Flush() return n, err }) @@ -75,7 +72,7 @@ func TestStreamWriteBlockedByOutputBuffer(t *testing.T) { }) if n, err := w.result(); n != len(want)-writeBufferSize || err != nil { - t.Fatalf("s.WriteContext() = %v, %v; want %v, nil", + t.Fatalf("s.Write() = %v, %v; want %v, nil", len(want)-writeBufferSize, err, writeBufferSize) } }) @@ -99,7 +96,7 @@ func TestStreamWriteBlockedByStreamFlowControl(t *testing.T) { } // Data is written to the stream output buffer, but we have no flow control. - _, err = s.WriteContext(ctx, want[:1]) + _, err = s.Write(want[:1]) if err != nil { t.Fatalf("write with available output buffer: unexpected error: %v", err) } @@ -110,7 +107,7 @@ func TestStreamWriteBlockedByStreamFlowControl(t *testing.T) { }) // Write more data. - _, err = s.WriteContext(ctx, want[1:]) + _, err = s.Write(want[1:]) if err != nil { t.Fatalf("write with available output buffer: unexpected error: %v", err) } @@ -172,7 +169,7 @@ func TestStreamIgnoresMaxStreamDataReduction(t *testing.T) { if err != nil { t.Fatal(err) } - s.WriteContext(ctx, want[:1]) + s.Write(want[:1]) s.Flush() tc.wantFrame("sent data (1 byte) fits within flow control limit", packetType1RTT, debugFrameStream{ @@ -188,7 +185,7 @@ func TestStreamIgnoresMaxStreamDataReduction(t *testing.T) { }) // Write [1,4). - s.WriteContext(ctx, want[1:]) + s.Write(want[1:]) tc.wantFrame("stream limit is 4 bytes, ignoring decrease in MAX_STREAM_DATA", packetType1RTT, debugFrameStream{ id: s.id, @@ -208,7 +205,7 @@ func TestStreamIgnoresMaxStreamDataReduction(t *testing.T) { }) // Write [1,4). - s.WriteContext(ctx, want[4:]) + s.Write(want[4:]) tc.wantFrame("stream limit is 8 bytes, ignoring decrease in MAX_STREAM_DATA", packetType1RTT, debugFrameStream{ id: s.id, @@ -220,7 +217,6 @@ func TestStreamIgnoresMaxStreamDataReduction(t *testing.T) { func TestStreamWriteBlockedByWriteBufferLimit(t *testing.T) { testStreamTypes(t, "", func(t *testing.T, styp streamType) { - ctx := canceledContext() want := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} const maxWriteBuffer = 4 tc := newTestConn(t, clientSide, func(p *transportParameters) { @@ -238,12 +234,10 @@ func TestStreamWriteBlockedByWriteBufferLimit(t *testing.T) { // Write more data than StreamWriteBufferSize. // The peer has given us plenty of flow control, // so we're just blocked by our local limit. - s, err := tc.conn.newLocalStream(ctx, styp) - if err != nil { - t.Fatal(err) - } + s := newLocalStream(t, tc, styp) w := runAsync(tc, func(ctx context.Context) (int, error) { - return s.WriteContext(ctx, want) + s.SetWriteContext(ctx) + return s.Write(want) }) tc.wantFrame("stream write should send as much data as write buffer allows", packetType1RTT, debugFrameStream{ @@ -266,7 +260,7 @@ func TestStreamWriteBlockedByWriteBufferLimit(t *testing.T) { w.cancel() n, err := w.result() if n != 2*maxWriteBuffer || err == nil { - t.Fatalf("WriteContext() = %v, %v; want %v bytes, error", n, err, 2*maxWriteBuffer) + t.Fatalf("Write() = %v, %v; want %v bytes, error", n, err, 2*maxWriteBuffer) } }) } @@ -397,7 +391,6 @@ func TestStreamReceive(t *testing.T) { }}, }} { testStreamTypes(t, test.name, func(t *testing.T, styp streamType) { - ctx := canceledContext() tc := newTestConn(t, serverSide) tc.handshake() sid := newStreamID(clientSide, styp, 0) @@ -413,21 +406,17 @@ func TestStreamReceive(t *testing.T) { fin: f.fin, }) if s == nil { - var err error - s, err = tc.conn.AcceptStream(ctx) - if err != nil { - tc.t.Fatalf("conn.AcceptStream() = %v", err) - } + s = tc.acceptStream() } for { - n, err := s.ReadContext(ctx, got[total:]) - t.Logf("s.ReadContext() = %v, %v", n, err) + n, err := s.Read(got[total:]) + t.Logf("s.Read() = %v, %v", n, err) total += n if f.wantEOF && err != io.EOF { - t.Fatalf("ReadContext() error = %v; want io.EOF", err) + t.Fatalf("Read() error = %v; want io.EOF", err) } if !f.wantEOF && err == io.EOF { - t.Fatalf("ReadContext() error = io.EOF, want something else") + t.Fatalf("Read() error = io.EOF, want something else") } if err != nil { break @@ -468,8 +457,8 @@ func TestStreamReceiveExtendsStreamWindow(t *testing.T) { } tc.wantIdle("stream window is not extended before data is read") buf := make([]byte, maxWindowSize+1) - if n, err := s.ReadContext(ctx, buf); n != maxWindowSize || err != nil { - t.Fatalf("s.ReadContext() = %v, %v; want %v, nil", n, err, maxWindowSize) + if n, err := s.Read(buf); n != maxWindowSize || err != nil { + t.Fatalf("s.Read() = %v, %v; want %v, nil", n, err, maxWindowSize) } tc.wantFrame("stream window is extended after reading data", packetType1RTT, debugFrameMaxStreamData{ @@ -482,8 +471,8 @@ func TestStreamReceiveExtendsStreamWindow(t *testing.T) { data: make([]byte, maxWindowSize), fin: true, }) - if n, err := s.ReadContext(ctx, buf); n != maxWindowSize || err != io.EOF { - t.Fatalf("s.ReadContext() = %v, %v; want %v, io.EOF", n, err, maxWindowSize) + if n, err := s.Read(buf); n != maxWindowSize || err != io.EOF { + t.Fatalf("s.Read() = %v, %v; want %v, io.EOF", n, err, maxWindowSize) } tc.wantIdle("stream window is not extended after FIN") }) @@ -673,18 +662,19 @@ func TestStreamReceiveUnblocksReader(t *testing.T) { t.Fatalf("AcceptStream() = %v", err) } - // ReadContext succeeds immediately, since we already have data. + // Read succeeds immediately, since we already have data. got := make([]byte, len(want)) read := runAsync(tc, func(ctx context.Context) (int, error) { - return s.ReadContext(ctx, got) + return s.Read(got) }) if n, err := read.result(); n != write1size || err != nil { - t.Fatalf("ReadContext = %v, %v; want %v, nil", n, err, write1size) + t.Fatalf("Read = %v, %v; want %v, nil", n, err, write1size) } - // ReadContext blocks waiting for more data. + // Read blocks waiting for more data. read = runAsync(tc, func(ctx context.Context) (int, error) { - return s.ReadContext(ctx, got[write1size:]) + s.SetReadContext(ctx) + return s.Read(got[write1size:]) }) tc.writeFrames(packetType1RTT, debugFrameStream{ id: sid, @@ -693,7 +683,7 @@ func TestStreamReceiveUnblocksReader(t *testing.T) { fin: true, }) if n, err := read.result(); n != len(want)-write1size || err != io.EOF { - t.Fatalf("ReadContext = %v, %v; want %v, io.EOF", n, err, len(want)-write1size) + t.Fatalf("Read = %v, %v; want %v, io.EOF", n, err, len(want)-write1size) } if !bytes.Equal(got, want) { t.Fatalf("read bytes %x, want %x", got, want) @@ -935,7 +925,8 @@ func TestStreamResetBlockedStream(t *testing.T) { }) tc.ignoreFrame(frameTypeStreamDataBlocked) writing := runAsync(tc, func(ctx context.Context) (int, error) { - return s.WriteContext(ctx, []byte{0, 1, 2, 3, 4, 5, 6, 7}) + s.SetWriteContext(ctx) + return s.Write([]byte{0, 1, 2, 3, 4, 5, 6, 7}) }) tc.wantFrame("stream writes data until write buffer fills", packetType1RTT, debugFrameStream{ @@ -972,7 +963,7 @@ func TestStreamWriteMoreThanOnePacketOfData(t *testing.T) { want := make([]byte, 4096) rand.Read(want) // doesn't need to be crypto/rand, but non-deprecated and harmless w := runAsync(tc, func(ctx context.Context) (int, error) { - n, err := s.WriteContext(ctx, want) + n, err := s.Write(want) s.Flush() return n, err }) @@ -992,7 +983,7 @@ func TestStreamWriteMoreThanOnePacketOfData(t *testing.T) { got = append(got, sf.data...) } if n, err := w.result(); n != len(want) || err != nil { - t.Fatalf("s.WriteContext() = %v, %v; want %v, nil", n, err, len(want)) + t.Fatalf("s.Write() = %v, %v; want %v, nil", n, err, len(want)) } if !bytes.Equal(got, want) { t.Fatalf("mismatch in received stream data") @@ -1000,17 +991,16 @@ func TestStreamWriteMoreThanOnePacketOfData(t *testing.T) { } func TestStreamCloseWaitsForAcks(t *testing.T) { - ctx := canceledContext() tc, s := newTestConnAndLocalStream(t, serverSide, uniStream, permissiveTransportParameters) data := make([]byte, 100) - s.WriteContext(ctx, data) + s.Write(data) s.Flush() tc.wantFrame("conn sends data for the stream", packetType1RTT, debugFrameStream{ id: s.id, data: data, }) - if err := s.CloseContext(ctx); err != context.Canceled { + if err := s.Close(); err != context.Canceled { t.Fatalf("s.Close() = %v, want context.Canceled (data not acked yet)", err) } tc.wantFrame("conn sends FIN for closed stream", @@ -1021,21 +1011,22 @@ func TestStreamCloseWaitsForAcks(t *testing.T) { data: []byte{}, }) closing := runAsync(tc, func(ctx context.Context) (struct{}, error) { - return struct{}{}, s.CloseContext(ctx) + s.SetWriteContext(ctx) + return struct{}{}, s.Close() }) if _, err := closing.result(); err != errNotDone { - t.Fatalf("s.CloseContext() = %v, want it to block waiting for acks", err) + t.Fatalf("s.Close() = %v, want it to block waiting for acks", err) } tc.writeAckForAll() if _, err := closing.result(); err != nil { - t.Fatalf("s.CloseContext() = %v, want nil (all data acked)", err) + t.Fatalf("s.Close() = %v, want nil (all data acked)", err) } } func TestStreamCloseReadOnly(t *testing.T) { tc, s := newTestConnAndRemoteStream(t, serverSide, uniStream, permissiveTransportParameters) - if err := s.CloseContext(canceledContext()); err != nil { - t.Errorf("s.CloseContext() = %v, want nil", err) + if err := s.Close(); err != nil { + t.Errorf("s.Close() = %v, want nil", err) } tc.wantFrame("closed stream sends STOP_SENDING", packetType1RTT, debugFrameStopSending{ @@ -1069,17 +1060,16 @@ func TestStreamCloseUnblocked(t *testing.T) { }, }} { t.Run(test.name, func(t *testing.T) { - ctx := canceledContext() tc, s := newTestConnAndLocalStream(t, serverSide, uniStream, permissiveTransportParameters) data := make([]byte, 100) - s.WriteContext(ctx, data) + s.Write(data) s.Flush() tc.wantFrame("conn sends data for the stream", packetType1RTT, debugFrameStream{ id: s.id, data: data, }) - if err := s.CloseContext(ctx); err != context.Canceled { + if err := s.Close(); err != context.Canceled { t.Fatalf("s.Close() = %v, want context.Canceled (data not acked yet)", err) } tc.wantFrame("conn sends FIN for closed stream", @@ -1090,34 +1080,34 @@ func TestStreamCloseUnblocked(t *testing.T) { data: []byte{}, }) closing := runAsync(tc, func(ctx context.Context) (struct{}, error) { - return struct{}{}, s.CloseContext(ctx) + s.SetWriteContext(ctx) + return struct{}{}, s.Close() }) if _, err := closing.result(); err != errNotDone { - t.Fatalf("s.CloseContext() = %v, want it to block waiting for acks", err) + t.Fatalf("s.Close() = %v, want it to block waiting for acks", err) } test.unblock(tc, s) _, err := closing.result() switch { case err == errNotDone: - t.Fatalf("s.CloseContext() still blocking; want it to have returned") + t.Fatalf("s.Close() still blocking; want it to have returned") case err == nil && !test.success: - t.Fatalf("s.CloseContext() = nil, want error") + t.Fatalf("s.Close() = nil, want error") case err != nil && test.success: - t.Fatalf("s.CloseContext() = %v, want nil (all data acked)", err) + t.Fatalf("s.Close() = %v, want nil (all data acked)", err) } }) } } func TestStreamCloseWriteWhenBlockedByStreamFlowControl(t *testing.T) { - ctx := canceledContext() tc, s := newTestConnAndLocalStream(t, serverSide, uniStream, permissiveTransportParameters, func(p *transportParameters) { //p.initialMaxData = 0 p.initialMaxStreamDataUni = 0 }) tc.ignoreFrame(frameTypeStreamDataBlocked) - if _, err := s.WriteContext(ctx, []byte{0, 1}); err != nil { + if _, err := s.Write([]byte{0, 1}); err != nil { t.Fatalf("s.Write = %v", err) } s.CloseWrite() @@ -1149,7 +1139,6 @@ func TestStreamCloseWriteWhenBlockedByStreamFlowControl(t *testing.T) { func TestStreamPeerResetsWithUnreadAndUnsentData(t *testing.T) { testStreamTypes(t, "", func(t *testing.T, styp streamType) { - ctx := canceledContext() tc, s := newTestConnAndRemoteStream(t, serverSide, styp) data := []byte{0, 1, 2, 3, 4, 5, 6, 7} tc.writeFrames(packetType1RTT, debugFrameStream{ @@ -1157,7 +1146,7 @@ func TestStreamPeerResetsWithUnreadAndUnsentData(t *testing.T) { data: data, }) got := make([]byte, 4) - if n, err := s.ReadContext(ctx, got); n != len(got) || err != nil { + if n, err := s.Read(got); n != len(got) || err != nil { t.Fatalf("Read start of stream: got %v, %v; want %v, nil", n, err, len(got)) } const sentCode = 42 @@ -1167,7 +1156,7 @@ func TestStreamPeerResetsWithUnreadAndUnsentData(t *testing.T) { code: sentCode, }) wantErr := StreamErrorCode(sentCode) - if n, err := s.ReadContext(ctx, got); n != 0 || !errors.Is(err, wantErr) { + if n, err := s.Read(got); n != 0 || !errors.Is(err, wantErr) { t.Fatalf("Read reset stream: got %v, %v; want 0, %v", n, err, wantErr) } }) @@ -1177,8 +1166,9 @@ func TestStreamPeerResetWakesBlockedRead(t *testing.T) { testStreamTypes(t, "", func(t *testing.T, styp streamType) { tc, s := newTestConnAndRemoteStream(t, serverSide, styp) reader := runAsync(tc, func(ctx context.Context) (int, error) { + s.SetReadContext(ctx) got := make([]byte, 4) - return s.ReadContext(ctx, got) + return s.Read(got) }) const sentCode = 42 tc.writeFrames(packetType1RTT, debugFrameResetStream{ @@ -1348,7 +1338,8 @@ func TestStreamFlushImplicitLargerThanBuffer(t *testing.T) { want := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} w := runAsync(tc, func(ctx context.Context) (int, error) { - n, err := s.WriteContext(ctx, want) + s.SetWriteContext(ctx) + n, err := s.Write(want) return n, err }) @@ -1401,7 +1392,10 @@ func newTestConnAndLocalStream(t *testing.T, side connSide, styp streamType, opt tc := newTestConn(t, side, opts...) tc.handshake() tc.ignoreFrame(frameTypeAck) - return tc, newLocalStream(t, tc, styp) + s := newLocalStream(t, tc, styp) + s.SetReadContext(canceledContext()) + s.SetWriteContext(canceledContext()) + return tc, s } func newLocalStream(t *testing.T, tc *testConn, styp streamType) *Stream { @@ -1411,6 +1405,8 @@ func newLocalStream(t *testing.T, tc *testConn, styp streamType) *Stream { if err != nil { t.Fatalf("conn.newLocalStream(%v) = %v", styp, err) } + s.SetReadContext(canceledContext()) + s.SetWriteContext(canceledContext()) return s } @@ -1419,7 +1415,10 @@ func newTestConnAndRemoteStream(t *testing.T, side connSide, styp streamType, op tc := newTestConn(t, side, opts...) tc.handshake() tc.ignoreFrame(frameTypeAck) - return tc, newRemoteStream(t, tc, styp) + s := newRemoteStream(t, tc, styp) + s.SetReadContext(canceledContext()) + s.SetWriteContext(canceledContext()) + return tc, s } func newRemoteStream(t *testing.T, tc *testConn, styp streamType) *Stream { @@ -1432,6 +1431,8 @@ func newRemoteStream(t *testing.T, tc *testConn, styp streamType) *Stream { if err != nil { t.Fatalf("conn.AcceptStream() = %v", err) } + s.SetReadContext(canceledContext()) + s.SetWriteContext(canceledContext()) return s } From 840656f9213922d0bb729d201162410b0bd74d9b Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 13 Feb 2024 15:36:02 -0800 Subject: [PATCH 02/22] quic/qlog: don't output empty slog.Attrs For golang/go#58547 Change-Id: I49a27ab82781c817511c6f7da0268529abc3f27f Reviewed-on: https://go-review.googlesource.com/c/net/+/564015 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/qlog/json_writer.go | 6 +++--- internal/quic/qlog/json_writer_test.go | 9 +++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/internal/quic/qlog/json_writer.go b/internal/quic/qlog/json_writer.go index b2fa3e03e..6fb8d33b2 100644 --- a/internal/quic/qlog/json_writer.go +++ b/internal/quic/qlog/json_writer.go @@ -45,15 +45,15 @@ func (w *jsonWriter) writeRecordEnd() { func (w *jsonWriter) writeAttrs(attrs []slog.Attr) { w.buf.WriteByte('{') for _, a := range attrs { - if a.Key == "" { - continue - } w.writeAttr(a) } w.buf.WriteByte('}') } func (w *jsonWriter) writeAttr(a slog.Attr) { + if a.Key == "" { + return + } w.writeName(a.Key) w.writeValue(a.Value) } diff --git a/internal/quic/qlog/json_writer_test.go b/internal/quic/qlog/json_writer_test.go index 6da556641..03cf6947c 100644 --- a/internal/quic/qlog/json_writer_test.go +++ b/internal/quic/qlog/json_writer_test.go @@ -85,6 +85,15 @@ func TestJSONWriterAttrs(t *testing.T) { `}}`) } +func TestJSONWriterAttrEmpty(t *testing.T) { + w := newTestJSONWriter() + w.writeRecordStart() + var a slog.Attr + w.writeAttr(a) + w.writeRecordEnd() + wantJSONRecord(t, w, `{}`) +} + func TestJSONWriterObjectEmpty(t *testing.T) { w := newTestJSONWriter() w.writeRecordStart() From 6e383c4aaf0635c980378ed3217f2a65391895a5 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 17 Nov 2023 08:06:43 -0800 Subject: [PATCH 03/22] quic: add qlog recovery metrics Log events for various congestion control and loss recovery metrics. For golang/go#58547 Change-Id: Ife3b3897f6ca731049c78b934a7123aa1ed4aee2 Reviewed-on: https://go-review.googlesource.com/c/net/+/564016 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- internal/quic/congestion_reno.go | 59 +++++++++++++++++++-- internal/quic/congestion_reno_test.go | 6 +-- internal/quic/conn.go | 2 +- internal/quic/conn_loss.go | 4 ++ internal/quic/conn_recv.go | 4 +- internal/quic/conn_send.go | 21 +++++--- internal/quic/loss.go | 47 ++++++++++++++--- internal/quic/loss_test.go | 10 ++-- internal/quic/packet_writer.go | 7 +-- internal/quic/qlog.go | 16 +++++- internal/quic/qlog_test.go | 76 ++++++++++++++++++++++++++- internal/quic/sent_packet.go | 7 +-- 12 files changed, 222 insertions(+), 37 deletions(-) diff --git a/internal/quic/congestion_reno.go b/internal/quic/congestion_reno.go index 982cbf4bb..a53983524 100644 --- a/internal/quic/congestion_reno.go +++ b/internal/quic/congestion_reno.go @@ -7,6 +7,8 @@ package quic import ( + "context" + "log/slog" "math" "time" ) @@ -40,6 +42,9 @@ type ccReno struct { // true if we haven't sent that packet yet. sendOnePacketInRecovery bool + // inRecovery is set when we are in the recovery state. + inRecovery bool + // underutilized is set if the congestion window is underutilized // due to insufficient application data, flow control limits, or // anti-amplification limits. @@ -100,12 +105,19 @@ func (c *ccReno) canSend() bool { // congestion controller permits sending data, but no data is sent. // // https://www.rfc-editor.org/rfc/rfc9002#section-7.8 -func (c *ccReno) setUnderutilized(v bool) { +func (c *ccReno) setUnderutilized(log *slog.Logger, v bool) { + if c.underutilized == v { + return + } + oldState := c.state() c.underutilized = v + if logEnabled(log, QLogLevelPacket) { + logCongestionStateUpdated(log, oldState, c.state()) + } } // packetSent indicates that a packet has been sent. -func (c *ccReno) packetSent(now time.Time, space numberSpace, sent *sentPacket) { +func (c *ccReno) packetSent(now time.Time, log *slog.Logger, space numberSpace, sent *sentPacket) { if !sent.inFlight { return } @@ -185,7 +197,11 @@ func (c *ccReno) packetLost(now time.Time, space numberSpace, sent *sentPacket, } // packetBatchEnd is called at the end of processing a batch of acked or lost packets. -func (c *ccReno) packetBatchEnd(now time.Time, space numberSpace, rtt *rttState, maxAckDelay time.Duration) { +func (c *ccReno) packetBatchEnd(now time.Time, log *slog.Logger, space numberSpace, rtt *rttState, maxAckDelay time.Duration) { + if logEnabled(log, QLogLevelPacket) { + oldState := c.state() + defer func() { logCongestionStateUpdated(log, oldState, c.state()) }() + } if !c.ackLastLoss.IsZero() && !c.ackLastLoss.Before(c.recoveryStartTime) { // Enter the recovery state. // https://www.rfc-editor.org/rfc/rfc9002.html#section-7.3.2 @@ -196,8 +212,10 @@ func (c *ccReno) packetBatchEnd(now time.Time, space numberSpace, rtt *rttState, // Clear congestionPendingAcks to avoid increasing the congestion // window based on acks in a frame that sends us into recovery. c.congestionPendingAcks = 0 + c.inRecovery = true } else if c.congestionPendingAcks > 0 { // We are in slow start or congestion avoidance. + c.inRecovery = false if c.congestionWindow < c.slowStartThreshold { // When the congestion window is less than the slow start threshold, // we are in slow start and increase the window by the number of @@ -253,3 +271,38 @@ func (c *ccReno) minimumCongestionWindow() int { // https://www.rfc-editor.org/rfc/rfc9002.html#section-7.2-4 return 2 * c.maxDatagramSize } + +func logCongestionStateUpdated(log *slog.Logger, oldState, newState congestionState) { + if oldState == newState { + return + } + log.LogAttrs(context.Background(), QLogLevelPacket, + "recovery:congestion_state_updated", + slog.String("old", oldState.String()), + slog.String("new", newState.String()), + ) +} + +type congestionState string + +func (s congestionState) String() string { return string(s) } + +const ( + congestionSlowStart = congestionState("slow_start") + congestionCongestionAvoidance = congestionState("congestion_avoidance") + congestionApplicationLimited = congestionState("application_limited") + congestionRecovery = congestionState("recovery") +) + +func (c *ccReno) state() congestionState { + switch { + case c.inRecovery: + return congestionRecovery + case c.underutilized: + return congestionApplicationLimited + case c.congestionWindow < c.slowStartThreshold: + return congestionSlowStart + default: + return congestionCongestionAvoidance + } +} diff --git a/internal/quic/congestion_reno_test.go b/internal/quic/congestion_reno_test.go index e9af6452c..cda7a90a8 100644 --- a/internal/quic/congestion_reno_test.go +++ b/internal/quic/congestion_reno_test.go @@ -470,7 +470,7 @@ func (c *ccTest) setRTT(smoothedRTT, rttvar time.Duration) { func (c *ccTest) setUnderutilized(v bool) { c.t.Helper() c.t.Logf("set underutilized = %v", v) - c.cc.setUnderutilized(v) + c.cc.setUnderutilized(nil, v) } func (c *ccTest) packetSent(space numberSpace, size int, fns ...func(*sentPacket)) *sentPacket { @@ -488,7 +488,7 @@ func (c *ccTest) packetSent(space numberSpace, size int, fns ...func(*sentPacket f(sent) } c.t.Logf("packet sent: num=%v.%v, size=%v", space, sent.num, sent.size) - c.cc.packetSent(c.now, space, sent) + c.cc.packetSent(c.now, nil, space, sent) return sent } @@ -519,7 +519,7 @@ func (c *ccTest) packetDiscarded(space numberSpace, sent *sentPacket) { func (c *ccTest) packetBatchEnd(space numberSpace) { c.t.Helper() c.t.Logf("(end of batch)") - c.cc.packetBatchEnd(c.now, space, &c.rtt, c.maxAckDelay) + c.cc.packetBatchEnd(c.now, nil, space, &c.rtt, c.maxAckDelay) } func (c *ccTest) wantCanSend(want bool) { diff --git a/internal/quic/conn.go b/internal/quic/conn.go index 6d79013eb..020bc81a4 100644 --- a/internal/quic/conn.go +++ b/internal/quic/conn.go @@ -210,7 +210,7 @@ func (c *Conn) discardKeys(now time.Time, space numberSpace) { case handshakeSpace: c.keysHandshake.discard() } - c.loss.discardKeys(now, space) + c.loss.discardKeys(now, c.log, space) } // receiveTransportParameters applies transport parameters sent by the peer. diff --git a/internal/quic/conn_loss.go b/internal/quic/conn_loss.go index 85bda314e..623ebdd7c 100644 --- a/internal/quic/conn_loss.go +++ b/internal/quic/conn_loss.go @@ -20,6 +20,10 @@ import "fmt" // See RFC 9000, Section 13.3 for a complete list of information which is retransmitted on loss. // https://www.rfc-editor.org/rfc/rfc9000#section-13.3 func (c *Conn) handleAckOrLoss(space numberSpace, sent *sentPacket, fate packetFate) { + if fate == packetLost && c.logEnabled(QLogLevelPacket) { + c.logPacketLost(space, sent) + } + // The list of frames in a sent packet is marshaled into a buffer in the sentPacket // by the packetWriter. Unmarshal that buffer here. This code must be kept in sync with // packetWriter.append*. diff --git a/internal/quic/conn_recv.go b/internal/quic/conn_recv.go index 045bf861c..b666ce8eb 100644 --- a/internal/quic/conn_recv.go +++ b/internal/quic/conn_recv.go @@ -192,7 +192,7 @@ func (c *Conn) handleRetry(now time.Time, pkt []byte) { c.connIDState.handleRetryPacket(p.srcConnID) // We need to resend any data we've already sent in Initial packets. // We must not reuse already sent packet numbers. - c.loss.discardPackets(initialSpace, c.handleAckOrLoss) + c.loss.discardPackets(initialSpace, c.log, c.handleAckOrLoss) // TODO: Discard 0-RTT packets as well, once we support 0-RTT. } @@ -416,7 +416,7 @@ func (c *Conn) handleAckFrame(now time.Time, space numberSpace, payload []byte) if c.peerAckDelayExponent >= 0 { delay = ackDelay.Duration(uint8(c.peerAckDelayExponent)) } - c.loss.receiveAckEnd(now, space, delay, c.handleAckOrLoss) + c.loss.receiveAckEnd(now, c.log, space, delay, c.handleAckOrLoss) if space == appDataSpace { c.keysAppData.handleAckFor(largest) } diff --git a/internal/quic/conn_send.go b/internal/quic/conn_send.go index ccb467591..575b8f9b4 100644 --- a/internal/quic/conn_send.go +++ b/internal/quic/conn_send.go @@ -22,7 +22,10 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { // Assumption: The congestion window is not underutilized. // If congestion control, pacing, and anti-amplification all permit sending, // but we have no packet to send, then we will declare the window underutilized. - c.loss.cc.setUnderutilized(false) + underutilized := false + defer func() { + c.loss.cc.setUnderutilized(c.log, underutilized) + }() // Send one datagram on each iteration of this loop, // until we hit a limit or run out of data to send. @@ -80,7 +83,6 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { } sentInitial = c.w.finishProtectedLongHeaderPacket(pnumMaxAcked, c.keysInitial.w, p) if sentInitial != nil { - c.idleHandlePacketSent(now, sentInitial) // Client initial packets and ack-eliciting server initial packaets // need to be sent in a datagram padded to at least 1200 bytes. // We can't add the padding yet, however, since we may want to @@ -111,8 +113,7 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { c.logPacketSent(packetTypeHandshake, pnum, p.srcConnID, p.dstConnID, c.w.packetLen(), c.w.payload()) } if sent := c.w.finishProtectedLongHeaderPacket(pnumMaxAcked, c.keysHandshake.w, p); sent != nil { - c.idleHandlePacketSent(now, sent) - c.loss.packetSent(now, handshakeSpace, sent) + c.packetSent(now, handshakeSpace, sent) if c.side == clientSide { // "[...] a client MUST discard Initial keys when it first // sends a Handshake packet [...]" @@ -142,8 +143,7 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { c.logPacketSent(packetType1RTT, pnum, nil, dstConnID, c.w.packetLen(), c.w.payload()) } if sent := c.w.finish1RTTPacket(pnum, pnumMaxAcked, dstConnID, &c.keysAppData); sent != nil { - c.idleHandlePacketSent(now, sent) - c.loss.packetSent(now, appDataSpace, sent) + c.packetSent(now, appDataSpace, sent) } } @@ -152,7 +152,7 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { if limit == ccOK { // We have nothing to send, and congestion control does not // block sending. The congestion window is underutilized. - c.loss.cc.setUnderutilized(true) + underutilized = true } return next } @@ -175,7 +175,7 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { // with a Handshake packet, then we've discarded Initial keys // since constructing the packet and shouldn't record it as in-flight. if c.keysInitial.canWrite() { - c.loss.packetSent(now, initialSpace, sentInitial) + c.packetSent(now, initialSpace, sentInitial) } } @@ -183,6 +183,11 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { } } +func (c *Conn) packetSent(now time.Time, space numberSpace, sent *sentPacket) { + c.idleHandlePacketSent(now, sent) + c.loss.packetSent(now, c.log, space, sent) +} + func (c *Conn) appendFrames(now time.Time, space numberSpace, pnum packetNumber, limit ccLimit) { if c.lifetime.localErr != nil { c.appendConnectionCloseFrame(now, space, c.lifetime.localErr) diff --git a/internal/quic/loss.go b/internal/quic/loss.go index a59081fd5..796b5f7a3 100644 --- a/internal/quic/loss.go +++ b/internal/quic/loss.go @@ -7,6 +7,8 @@ package quic import ( + "context" + "log/slog" "math" "time" ) @@ -179,7 +181,7 @@ func (c *lossState) nextNumber(space numberSpace) packetNumber { } // packetSent records a sent packet. -func (c *lossState) packetSent(now time.Time, space numberSpace, sent *sentPacket) { +func (c *lossState) packetSent(now time.Time, log *slog.Logger, space numberSpace, sent *sentPacket) { sent.time = now c.spaces[space].add(sent) size := sent.size @@ -187,13 +189,16 @@ func (c *lossState) packetSent(now time.Time, space numberSpace, sent *sentPacke c.antiAmplificationLimit = max(0, c.antiAmplificationLimit-size) } if sent.inFlight { - c.cc.packetSent(now, space, sent) + c.cc.packetSent(now, log, space, sent) c.pacer.packetSent(now, size, c.cc.congestionWindow, c.rtt.smoothedRTT) if sent.ackEliciting { c.spaces[space].lastAckEliciting = sent.num c.ptoExpired = false // reset expired PTO timer after sending probe } c.scheduleTimer(now) + if logEnabled(log, QLogLevelPacket) { + logBytesInFlight(log, c.cc.bytesInFlight) + } } if sent.ackEliciting { c.consecutiveNonAckElicitingPackets = 0 @@ -267,7 +272,7 @@ func (c *lossState) receiveAckRange(now time.Time, space numberSpace, rangeIndex // receiveAckEnd finishes processing an ack frame. // The lossf function is called for each packet newly detected as lost. -func (c *lossState) receiveAckEnd(now time.Time, space numberSpace, ackDelay time.Duration, lossf func(numberSpace, *sentPacket, packetFate)) { +func (c *lossState) receiveAckEnd(now time.Time, log *slog.Logger, space numberSpace, ackDelay time.Duration, lossf func(numberSpace, *sentPacket, packetFate)) { c.spaces[space].sentPacketList.clean() // Update the RTT sample when the largest acknowledged packet in the ACK frame // is newly acknowledged, and at least one newly acknowledged packet is ack-eliciting. @@ -286,13 +291,30 @@ func (c *lossState) receiveAckEnd(now time.Time, space numberSpace, ackDelay tim // https://www.rfc-editor.org/rfc/rfc9002.html#section-6.2.2.1-3 c.timer = time.Time{} c.detectLoss(now, lossf) - c.cc.packetBatchEnd(now, space, &c.rtt, c.maxAckDelay) + c.cc.packetBatchEnd(now, log, space, &c.rtt, c.maxAckDelay) + + if logEnabled(log, QLogLevelPacket) { + var ssthresh slog.Attr + if c.cc.slowStartThreshold != math.MaxInt { + ssthresh = slog.Int("ssthresh", c.cc.slowStartThreshold) + } + log.LogAttrs(context.Background(), QLogLevelPacket, + "recovery:metrics_updated", + slog.Duration("min_rtt", c.rtt.minRTT), + slog.Duration("smoothed_rtt", c.rtt.smoothedRTT), + slog.Duration("latest_rtt", c.rtt.latestRTT), + slog.Duration("rtt_variance", c.rtt.rttvar), + slog.Int("congestion_window", c.cc.congestionWindow), + slog.Int("bytes_in_flight", c.cc.bytesInFlight), + ssthresh, + ) + } } // discardPackets declares that packets within a number space will not be delivered // and that data contained in them should be resent. // For example, after receiving a Retry packet we discard already-sent Initial packets. -func (c *lossState) discardPackets(space numberSpace, lossf func(numberSpace, *sentPacket, packetFate)) { +func (c *lossState) discardPackets(space numberSpace, log *slog.Logger, lossf func(numberSpace, *sentPacket, packetFate)) { for i := 0; i < c.spaces[space].size; i++ { sent := c.spaces[space].nth(i) sent.lost = true @@ -300,10 +322,13 @@ func (c *lossState) discardPackets(space numberSpace, lossf func(numberSpace, *s lossf(numberSpace(space), sent, packetLost) } c.spaces[space].clean() + if logEnabled(log, QLogLevelPacket) { + logBytesInFlight(log, c.cc.bytesInFlight) + } } // discardKeys is called when dropping packet protection keys for a number space. -func (c *lossState) discardKeys(now time.Time, space numberSpace) { +func (c *lossState) discardKeys(now time.Time, log *slog.Logger, space numberSpace) { // https://www.rfc-editor.org/rfc/rfc9002.html#section-6.4 for i := 0; i < c.spaces[space].size; i++ { sent := c.spaces[space].nth(i) @@ -313,6 +338,9 @@ func (c *lossState) discardKeys(now time.Time, space numberSpace) { c.spaces[space].maxAcked = -1 c.spaces[space].lastAckEliciting = -1 c.scheduleTimer(now) + if logEnabled(log, QLogLevelPacket) { + logBytesInFlight(log, c.cc.bytesInFlight) + } } func (c *lossState) lossDuration() time.Duration { @@ -459,3 +487,10 @@ func (c *lossState) ptoBasePeriod() time.Duration { } return pto } + +func logBytesInFlight(log *slog.Logger, bytesInFlight int) { + log.LogAttrs(context.Background(), QLogLevelPacket, + "recovery:metrics_updated", + slog.Int("bytes_in_flight", bytesInFlight), + ) +} diff --git a/internal/quic/loss_test.go b/internal/quic/loss_test.go index efbf1649e..1fb9662e4 100644 --- a/internal/quic/loss_test.go +++ b/internal/quic/loss_test.go @@ -1060,7 +1060,7 @@ func TestLossPersistentCongestion(t *testing.T) { maxDatagramSize: 1200, }) test.send(initialSpace, 0, testSentPacketSize(1200)) - test.c.cc.setUnderutilized(true) + test.c.cc.setUnderutilized(nil, true) test.advance(10 * time.Millisecond) test.ack(initialSpace, 0*time.Millisecond, i64range[packetNumber]{0, 1}) @@ -1377,7 +1377,7 @@ func (c *lossTest) setRTTVar(d time.Duration) { func (c *lossTest) setUnderutilized(v bool) { c.t.Logf("set congestion window underutilized: %v", v) - c.c.cc.setUnderutilized(v) + c.c.cc.setUnderutilized(nil, v) } func (c *lossTest) advance(d time.Duration) { @@ -1438,7 +1438,7 @@ func (c *lossTest) send(spaceID numberSpace, opts ...any) { sent := &sentPacket{} *sent = prototype sent.num = num - c.c.packetSent(c.now, spaceID, sent) + c.c.packetSent(c.now, nil, spaceID, sent) } } @@ -1462,7 +1462,7 @@ func (c *lossTest) ack(spaceID numberSpace, ackDelay time.Duration, rs ...i64ran c.t.Logf("ack %v delay=%v [%v,%v)", spaceID, ackDelay, r.start, r.end) c.c.receiveAckRange(c.now, spaceID, i, r.start, r.end, c.onAckOrLoss) } - c.c.receiveAckEnd(c.now, spaceID, ackDelay, c.onAckOrLoss) + c.c.receiveAckEnd(c.now, nil, spaceID, ackDelay, c.onAckOrLoss) } func (c *lossTest) onAckOrLoss(space numberSpace, sent *sentPacket, fate packetFate) { @@ -1491,7 +1491,7 @@ func (c *lossTest) discardKeys(spaceID numberSpace) { c.t.Helper() c.checkUnexpectedEvents() c.t.Logf("discard %s keys", spaceID) - c.c.discardKeys(c.now, spaceID) + c.c.discardKeys(c.now, nil, spaceID) } func (c *lossTest) setMaxAckDelay(d time.Duration) { diff --git a/internal/quic/packet_writer.go b/internal/quic/packet_writer.go index b4e54ce4b..85149f607 100644 --- a/internal/quic/packet_writer.go +++ b/internal/quic/packet_writer.go @@ -141,7 +141,7 @@ func (w *packetWriter) finishProtectedLongHeaderPacket(pnumMaxAcked packetNumber hdr = appendPacketNumber(hdr, p.num, pnumMaxAcked) k.protect(hdr[w.pktOff:], w.b[len(hdr):], pnumOff-w.pktOff, p.num) - return w.finish(p.num) + return w.finish(p.ptype, p.num) } // start1RTTPacket starts writing a 1-RTT (short header) packet. @@ -183,7 +183,7 @@ func (w *packetWriter) finish1RTTPacket(pnum, pnumMaxAcked packetNumber, dstConn hdr = appendPacketNumber(hdr, pnum, pnumMaxAcked) w.padPacketLength(pnumLen) k.protect(hdr[w.pktOff:], w.b[len(hdr):], pnumOff-w.pktOff, pnum) - return w.finish(pnum) + return w.finish(packetType1RTT, pnum) } // padPacketLength pads out the payload of the current packet to the minimum size, @@ -204,9 +204,10 @@ func (w *packetWriter) padPacketLength(pnumLen int) int { } // finish finishes the current packet after protection is applied. -func (w *packetWriter) finish(pnum packetNumber) *sentPacket { +func (w *packetWriter) finish(ptype packetType, pnum packetNumber) *sentPacket { w.b = w.b[:len(w.b)+aeadOverhead] w.sent.size = len(w.b) - w.pktOff + w.sent.ptype = ptype w.sent.num = pnum sent := w.sent w.sent = nil diff --git a/internal/quic/qlog.go b/internal/quic/qlog.go index 82ad92ac8..e37e2f8ce 100644 --- a/internal/quic/qlog.go +++ b/internal/quic/qlog.go @@ -39,7 +39,11 @@ const ( ) func (c *Conn) logEnabled(level slog.Level) bool { - return c.log != nil && c.log.Enabled(context.Background(), level) + return logEnabled(c.log, level) +} + +func logEnabled(log *slog.Logger, level slog.Level) bool { + return log != nil && log.Enabled(context.Background(), level) } // slogHexstring returns a slog.Attr for a value of the hexstring type. @@ -252,3 +256,13 @@ func (c *Conn) packetFramesAttr(payload []byte) slog.Attr { } return slog.Any("frames", frames) } + +func (c *Conn) logPacketLost(space numberSpace, sent *sentPacket) { + c.log.LogAttrs(context.Background(), QLogLevelPacket, + "recovery:packet_lost", + slog.Group("header", + slog.String("packet_type", sent.ptype.qlogString()), + slog.Uint64("packet_number", uint64(sent.num)), + ), + ) +} diff --git a/internal/quic/qlog_test.go b/internal/quic/qlog_test.go index e98b11838..7ad65524c 100644 --- a/internal/quic/qlog_test.go +++ b/internal/quic/qlog_test.go @@ -159,6 +159,77 @@ func TestQLogConnectionClosedTrigger(t *testing.T) { } } +func TestQLogRecovery(t *testing.T) { + qr := &qlogRecord{} + tc, s := newTestConnAndLocalStream(t, clientSide, uniStream, + permissiveTransportParameters, qr.config) + + // Ignore events from the handshake. + qr.ev = nil + + data := make([]byte, 16) + s.Write(data) + s.CloseWrite() + tc.wantFrame("created stream 0", + packetType1RTT, debugFrameStream{ + id: newStreamID(clientSide, uniStream, 0), + fin: true, + data: data, + }) + tc.writeAckForAll() + tc.wantIdle("connection should be idle now") + + // Don't check the contents of fields, but verify that recovery metrics are logged. + qr.wantEvents(t, jsonEvent{ + "name": "recovery:metrics_updated", + "data": map[string]any{ + "bytes_in_flight": nil, + }, + }, jsonEvent{ + "name": "recovery:metrics_updated", + "data": map[string]any{ + "bytes_in_flight": 0, + "congestion_window": nil, + "latest_rtt": nil, + "min_rtt": nil, + "rtt_variance": nil, + "smoothed_rtt": nil, + }, + }) +} + +func TestQLogLoss(t *testing.T) { + qr := &qlogRecord{} + tc, s := newTestConnAndLocalStream(t, clientSide, uniStream, + permissiveTransportParameters, qr.config) + + // Ignore events from the handshake. + qr.ev = nil + + data := make([]byte, 16) + s.Write(data) + s.CloseWrite() + tc.wantFrame("created stream 0", + packetType1RTT, debugFrameStream{ + id: newStreamID(clientSide, uniStream, 0), + fin: true, + data: data, + }) + + const pto = false + tc.triggerLossOrPTO(packetType1RTT, pto) + + qr.wantEvents(t, jsonEvent{ + "name": "recovery:packet_lost", + "data": map[string]any{ + "header": map[string]any{ + "packet_number": nil, + "packet_type": "1RTT", + }, + }, + }) +} + type nopCloseWriter struct { io.Writer } @@ -193,14 +264,15 @@ func jsonPartialEqual(got, want any) (equal bool) { } return v } + if want == nil { + return true // match anything + } got = cmpval(got) want = cmpval(want) if reflect.TypeOf(got) != reflect.TypeOf(want) { return false } switch w := want.(type) { - case nil: - // Match anything. case map[string]any: // JSON object: Every field in want must match a field in got. g := got.(map[string]any) diff --git a/internal/quic/sent_packet.go b/internal/quic/sent_packet.go index 4f11aa136..194cdc9fa 100644 --- a/internal/quic/sent_packet.go +++ b/internal/quic/sent_packet.go @@ -14,9 +14,10 @@ import ( // A sentPacket tracks state related to an in-flight packet we sent, // to be committed when the peer acks it or resent if the packet is lost. type sentPacket struct { - num packetNumber - size int // size in bytes - time time.Time // time sent + num packetNumber + size int // size in bytes + time time.Time // time sent + ptype packetType ackEliciting bool // https://www.rfc-editor.org/rfc/rfc9002.html#section-2-3.4.1 inFlight bool // https://www.rfc-editor.org/rfc/rfc9002.html#section-2-3.6.1 From 93be8fe122ca52e008630144471e9473d94cc43f Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 28 Nov 2023 09:19:01 -0800 Subject: [PATCH 04/22] quic: log packet_dropped events Log unparsable or otherwise discarded packets. For golang/go#58547 Change-Id: Ief64174d91c93691bd524515aa6518e487543ced Reviewed-on: https://go-review.googlesource.com/c/net/+/564017 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/conn_recv.go | 15 ++++++++++++--- internal/quic/qlog_test.go | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/internal/quic/conn_recv.go b/internal/quic/conn_recv.go index b666ce8eb..1b3219723 100644 --- a/internal/quic/conn_recv.go +++ b/internal/quic/conn_recv.go @@ -8,6 +8,7 @@ package quic import ( "bytes" + "context" "encoding/binary" "errors" "time" @@ -56,9 +57,16 @@ func (c *Conn) handleDatagram(now time.Time, dgram *datagram) { if len(buf) == len(dgram.b) && len(buf) > statelessResetTokenLen { var token statelessResetToken copy(token[:], buf[len(buf)-len(token):]) - c.handleStatelessReset(now, token) + if c.handleStatelessReset(now, token) { + return + } } // Invalid data at the end of a datagram is ignored. + if c.logEnabled(QLogLevelPacket) { + c.log.LogAttrs(context.Background(), QLogLevelPacket, + "connectivity:packet_dropped", + ) + } break } c.idleHandlePacketReceived(now) @@ -562,10 +570,11 @@ func (c *Conn) handleHandshakeDoneFrame(now time.Time, space numberSpace, payloa var errStatelessReset = errors.New("received stateless reset") -func (c *Conn) handleStatelessReset(now time.Time, resetToken statelessResetToken) { +func (c *Conn) handleStatelessReset(now time.Time, resetToken statelessResetToken) (valid bool) { if !c.connIDState.isValidStatelessResetToken(resetToken) { - return + return false } c.setFinalError(errStatelessReset) c.enterDraining(now) + return true } diff --git a/internal/quic/qlog_test.go b/internal/quic/qlog_test.go index 7ad65524c..6c79c6cf4 100644 --- a/internal/quic/qlog_test.go +++ b/internal/quic/qlog_test.go @@ -7,6 +7,7 @@ package quic import ( + "bytes" "encoding/hex" "encoding/json" "fmt" @@ -230,6 +231,27 @@ func TestQLogLoss(t *testing.T) { }) } +func TestQLogPacketDropped(t *testing.T) { + qr := &qlogRecord{} + tc := newTestConn(t, clientSide, permissiveTransportParameters, qr.config) + tc.handshake() + + // A garbage-filled datagram with a DCID matching this connection. + dgram := bytes.Join([][]byte{ + {headerFormShort | fixedBit}, + testLocalConnID(0), + make([]byte, 100), + []byte{1, 2, 3, 4}, // random data, to avoid this looking like a stateless reset + }, nil) + tc.endpoint.write(&datagram{ + b: dgram, + }) + + qr.wantEvents(t, jsonEvent{ + "name": "connectivity:packet_dropped", + }) +} + type nopCloseWriter struct { io.Writer } From 117945d00a55197e260d73c6272a2588d39bdebe Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 20 Nov 2023 16:05:25 -0800 Subject: [PATCH 05/22] quic: add throughput and stream creation benchmarks For golang/go#58547 Change-Id: Ie62fcf596bf020bda5a167f7a0d3d95bac9e591a Reviewed-on: https://go-review.googlesource.com/c/net/+/564475 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- internal/quic/bench_test.go | 99 ++++++++++++++++++++++++++++++++++ internal/quic/endpoint_test.go | 4 +- 2 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 internal/quic/bench_test.go diff --git a/internal/quic/bench_test.go b/internal/quic/bench_test.go new file mode 100644 index 000000000..f883b788c --- /dev/null +++ b/internal/quic/bench_test.go @@ -0,0 +1,99 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 + +package quic + +import ( + "context" + "fmt" + "io" + "math" + "testing" +) + +// BenchmarkThroughput is based on the crypto/tls benchmark of the same name. +func BenchmarkThroughput(b *testing.B) { + for size := 1; size <= 64; size <<= 1 { + name := fmt.Sprintf("%dMiB", size) + b.Run(name, func(b *testing.B) { + throughput(b, int64(size<<20)) + }) + } +} + +func throughput(b *testing.B, totalBytes int64) { + // Same buffer size as crypto/tls's BenchmarkThroughput, for consistency. + const bufsize = 32 << 10 + + cli, srv := newLocalConnPair(b, &Config{}, &Config{}) + + go func() { + buf := make([]byte, bufsize) + for i := 0; i < b.N; i++ { + sconn, err := srv.AcceptStream(context.Background()) + if err != nil { + panic(fmt.Errorf("AcceptStream: %v", err)) + } + if _, err := io.CopyBuffer(sconn, sconn, buf); err != nil { + panic(fmt.Errorf("CopyBuffer: %v", err)) + } + sconn.Close() + } + }() + + b.SetBytes(totalBytes) + buf := make([]byte, bufsize) + chunks := int(math.Ceil(float64(totalBytes) / float64(len(buf)))) + for i := 0; i < b.N; i++ { + cconn, err := cli.NewStream(context.Background()) + if err != nil { + b.Fatalf("NewStream: %v", err) + } + closec := make(chan struct{}) + go func() { + defer close(closec) + buf := make([]byte, bufsize) + if _, err := io.CopyBuffer(io.Discard, cconn, buf); err != nil { + panic(fmt.Errorf("Discard: %v", err)) + } + }() + for j := 0; j < chunks; j++ { + _, err := cconn.Write(buf) + if err != nil { + b.Fatalf("Write: %v", err) + } + } + cconn.CloseWrite() + <-closec + cconn.Close() + } +} + +func BenchmarkStreamCreation(b *testing.B) { + cli, srv := newLocalConnPair(b, &Config{}, &Config{}) + + go func() { + for i := 0; i < b.N; i++ { + sconn, err := srv.AcceptStream(context.Background()) + if err != nil { + panic(fmt.Errorf("AcceptStream: %v", err)) + } + sconn.Close() + } + }() + + buf := make([]byte, 1) + for i := 0; i < b.N; i++ { + cconn, err := cli.NewStream(context.Background()) + if err != nil { + b.Fatalf("NewStream: %v", err) + } + cconn.Write(buf) + cconn.Flush() + cconn.Read(buf) + cconn.Close() + } +} diff --git a/internal/quic/endpoint_test.go b/internal/quic/endpoint_test.go index 16c3e0bce..6d103f061 100644 --- a/internal/quic/endpoint_test.go +++ b/internal/quic/endpoint_test.go @@ -63,7 +63,7 @@ func TestStreamTransfer(t *testing.T) { } } -func newLocalConnPair(t *testing.T, conf1, conf2 *Config) (clientConn, serverConn *Conn) { +func newLocalConnPair(t testing.TB, conf1, conf2 *Config) (clientConn, serverConn *Conn) { t.Helper() ctx := context.Background() e1 := newLocalEndpoint(t, serverSide, conf1) @@ -79,7 +79,7 @@ func newLocalConnPair(t *testing.T, conf1, conf2 *Config) (clientConn, serverCon return c2, c1 } -func newLocalEndpoint(t *testing.T, side connSide, conf *Config) *Endpoint { +func newLocalEndpoint(t testing.TB, side connSide, conf *Config) *Endpoint { t.Helper() if conf.TLSConfig == nil { newConf := *conf From e94da73eedb3c3244dcc3857c74accb642dd8eac Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 12 Dec 2023 15:48:00 -0800 Subject: [PATCH 06/22] quic: reduce ack frequency after the first 100 packets RFC 9000 recommends sending an ack for every second ack-eliciting packet received. This frequency is high enough to have a noticeable impact on performance. Follow the approach used by Google QUICHE: Ack every other packet for the first 100 packets, and then switch to acking every 10th packet. (Various other implementations also use a reduced ack frequency; see Custura et al., 2022.) For golang/go#58547 Change-Id: Idc7051cec23c279811030eb555bc49bb888d6795 Reviewed-on: https://go-review.googlesource.com/c/net/+/564476 Reviewed-by: Jonathan Amsterdam Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI --- internal/quic/acks.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/quic/acks.go b/internal/quic/acks.go index ba860efb2..039b7b46e 100644 --- a/internal/quic/acks.go +++ b/internal/quic/acks.go @@ -130,12 +130,19 @@ func (acks *ackState) mustAckImmediately(space numberSpace, num packetNumber) bo // there are no gaps. If it does not, there must be a gap. return true } - if acks.unackedAckEliciting >= 2 { - // "[...] after receiving at least two ack-eliciting packets." - // https://www.rfc-editor.org/rfc/rfc9000.html#section-13.2.2 - return true + // "[...] SHOULD send an ACK frame after receiving at least two ack-eliciting packets." + // https://www.rfc-editor.org/rfc/rfc9000.html#section-13.2.2 + // + // This ack frequency takes a substantial toll on performance, however. + // Follow the behavior of Google QUICHE: + // Ack every other packet for the first 100 packets, and then ack every 10th packet. + // This keeps ack frequency high during the beginning of slow start when CWND is + // increasing rapidly. + packetsBeforeAck := 2 + if acks.seen.max() > 100 { + packetsBeforeAck = 10 } - return false + return acks.unackedAckEliciting >= packetsBeforeAck } // shouldSendAck reports whether the connection should send an ACK frame at this time, From dda3687b193e5e1fb31df72be5e0bc6ae7841d2e Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 31 Oct 2023 14:06:26 -0700 Subject: [PATCH 07/22] quic: add Stream.ReadByte, Stream.WriteByte Currently unoptimized and slow. Adding along with a benchmark to compare to the fast-path followup. For golang/go#58547 Change-Id: If02b65e6e7cfc770d3f949e5fb9fbb9d8a765a90 Reviewed-on: https://go-review.googlesource.com/c/net/+/564477 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- internal/quic/bench_test.go | 71 +++++++++++++++++++++++++++++++++++++ internal/quic/stream.go | 14 ++++++++ 2 files changed, 85 insertions(+) diff --git a/internal/quic/bench_test.go b/internal/quic/bench_test.go index f883b788c..636b71327 100644 --- a/internal/quic/bench_test.go +++ b/internal/quic/bench_test.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "math" + "sync" "testing" ) @@ -72,6 +73,76 @@ func throughput(b *testing.B, totalBytes int64) { } } +func BenchmarkReadByte(b *testing.B) { + cli, srv := newLocalConnPair(b, &Config{}, &Config{}) + + var wg sync.WaitGroup + defer wg.Wait() + + wg.Add(1) + go func() { + defer wg.Done() + buf := make([]byte, 1<<20) + sconn, err := srv.AcceptStream(context.Background()) + if err != nil { + panic(fmt.Errorf("AcceptStream: %v", err)) + } + for { + if _, err := sconn.Write(buf); err != nil { + break + } + sconn.Flush() + } + }() + + b.SetBytes(1) + cconn, err := cli.NewStream(context.Background()) + if err != nil { + b.Fatalf("NewStream: %v", err) + } + cconn.Flush() + for i := 0; i < b.N; i++ { + _, err := cconn.ReadByte() + if err != nil { + b.Fatalf("ReadByte: %v", err) + } + } + cconn.Close() +} + +func BenchmarkWriteByte(b *testing.B) { + cli, srv := newLocalConnPair(b, &Config{}, &Config{}) + + var wg sync.WaitGroup + defer wg.Wait() + + wg.Add(1) + go func() { + defer wg.Done() + sconn, err := srv.AcceptStream(context.Background()) + if err != nil { + panic(fmt.Errorf("AcceptStream: %v", err)) + } + n, err := io.Copy(io.Discard, sconn) + if n != int64(b.N) || err != nil { + b.Errorf("server io.Copy() = %v, %v; want %v, nil", n, err, b.N) + } + }() + + b.SetBytes(1) + cconn, err := cli.NewStream(context.Background()) + if err != nil { + b.Fatalf("NewStream: %v", err) + } + cconn.Flush() + for i := 0; i < b.N; i++ { + if err := cconn.WriteByte(0); err != nil { + b.Fatalf("WriteByte: %v", err) + } + } + cconn.Close() +} + func BenchmarkStreamCreation(b *testing.B) { cli, srv := newLocalConnPair(b, &Config{}, &Config{}) diff --git a/internal/quic/stream.go b/internal/quic/stream.go index d0122b951..670b34263 100644 --- a/internal/quic/stream.go +++ b/internal/quic/stream.go @@ -245,6 +245,13 @@ func (s *Stream) Read(b []byte) (n int, err error) { return len(b), nil } +// ReadByte reads and returns a single byte from the stream. +func (s *Stream) ReadByte() (byte, error) { + var b [1]byte + _, err := s.Read(b[:]) + return b[0], err +} + // shouldUpdateFlowControl determines whether to send a flow control window update. // // We want to balance keeping the peer well-supplied with flow control with not sending @@ -326,6 +333,13 @@ func (s *Stream) Write(b []byte) (n int, err error) { return n, nil } +// WriteBytes writes a single byte to the stream. +func (s *Stream) WriteByte(c byte) error { + b := [1]byte{c} + _, err := s.Write(b[:]) + return err +} + // Flush flushes data written to the stream. // It does not wait for the peer to acknowledge receipt of the data. // Use Close to wait for the peer's acknowledgement. From cc568eace4e2768d6befe9748ee0f3cd4edd9a10 Mon Sep 17 00:00:00 2001 From: Tobias Klauser Date: Tue, 20 Feb 2024 12:29:30 +0100 Subject: [PATCH 08/22] internal/quic: use slices.Equal in TestAcksSent The module go.mod uses go 1.18 and acks_test.go has a go:build go1.21 tag. Change-Id: Ic0785bcb4795bedecc6a752f5e67a967851237e6 Reviewed-on: https://go-review.googlesource.com/c/net/+/565137 Reviewed-by: Than McIntosh Auto-Submit: Tobias Klauser Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/acks_test.go | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/internal/quic/acks_test.go b/internal/quic/acks_test.go index 4f1032910..d10f917ad 100644 --- a/internal/quic/acks_test.go +++ b/internal/quic/acks_test.go @@ -7,6 +7,7 @@ package quic import ( + "slices" "testing" "time" ) @@ -198,7 +199,7 @@ func TestAcksSent(t *testing.T) { if len(gotNums) == 0 { wantDelay = 0 } - if !slicesEqual(gotNums, test.wantAcks) || gotDelay != wantDelay { + if !slices.Equal(gotNums, test.wantAcks) || gotDelay != wantDelay { t.Errorf("acks.acksToSend(T+%v) = %v, %v; want %v, %v", delay, gotNums, gotDelay, test.wantAcks, wantDelay) } } @@ -206,20 +207,6 @@ func TestAcksSent(t *testing.T) { } } -// slicesEqual reports whether two slices are equal. -// Replace this with slices.Equal once the module go.mod is go1.17 or newer. -func slicesEqual[E comparable](s1, s2 []E) bool { - if len(s1) != len(s2) { - return false - } - for i := range s1 { - if s1[i] != s2[i] { - return false - } - } - return true -} - func TestAcksDiscardAfterAck(t *testing.T) { acks := ackState{} now := time.Now() From 08d27e39b9ef291f25ae7e4d34440c8d89d6b7f7 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 27 Nov 2023 09:07:49 -0800 Subject: [PATCH 09/22] quic: fast path for stream reads Keep a reference to the next chunk of bytes available for reading in an unsynchronized buffer. Read and ReadByte calls read from this buffer when possible, avoiding the need to lock the stream. This change makes it unnecessary to wrap a stream in a *bytes.Buffer when making small reads, at the expense of making reads concurrency-unsafe. Since the quic package is a low-level one and this lets us avoid an extra buffer in the HTTP/3 implementation, the tradeoff seems worthwhile. For golang/go#58547 Change-Id: Ib3ca446311974571c2367295b302f36a6349b00d Reviewed-on: https://go-review.googlesource.com/c/net/+/564495 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/conn_flow_test.go | 52 ++++++++++++------------- internal/quic/conn_loss_test.go | 20 +++++++--- internal/quic/pipe.go | 23 ++++++++--- internal/quic/stream.go | 67 +++++++++++++++++++++++++++------ internal/quic/stream_test.go | 30 ++++++++++++++- 5 files changed, 143 insertions(+), 49 deletions(-) diff --git a/internal/quic/conn_flow_test.go b/internal/quic/conn_flow_test.go index 8e04e20d9..260684bdb 100644 --- a/internal/quic/conn_flow_test.go +++ b/internal/quic/conn_flow_test.go @@ -17,33 +17,29 @@ func TestConnInflowReturnOnRead(t *testing.T) { }) tc.writeFrames(packetType1RTT, debugFrameStream{ id: s.id, - data: make([]byte, 64), + data: make([]byte, 8), }) - const readSize = 8 - if n, err := s.Read(make([]byte, readSize)); n != readSize || err != nil { - t.Fatalf("s.Read() = %v, %v; want %v, nil", n, err, readSize) - } - tc.wantFrame("available window increases, send a MAX_DATA", - packetType1RTT, debugFrameMaxData{ - max: 64 + readSize, - }) - if n, err := s.Read(make([]byte, 64)); n != 64-readSize || err != nil { - t.Fatalf("s.Read() = %v, %v; want %v, nil", n, err, 64-readSize) + if n, err := s.Read(make([]byte, 8)); n != 8 || err != nil { + t.Fatalf("s.Read() = %v, %v; want %v, nil", n, err, 8) } tc.wantFrame("available window increases, send a MAX_DATA", packetType1RTT, debugFrameMaxData{ - max: 128, + max: 64 + 8, }) // Peer can write up to the new limit. tc.writeFrames(packetType1RTT, debugFrameStream{ id: s.id, - off: 64, + off: 8, data: make([]byte, 64), }) - tc.wantIdle("connection is idle") - if n, err := s.Read(make([]byte, 64)); n != 64 || err != nil { - t.Fatalf("offset 64: s.Read() = %v, %v; want %v, nil", n, err, 64) + if n, err := s.Read(make([]byte, 64+1)); n != 64 { + t.Fatalf("s.Read() = %v, %v; want %v, anything", n, err, 64) } + tc.wantFrame("available window increases, send a MAX_DATA", + packetType1RTT, debugFrameMaxData{ + max: 64 + 8 + 64, + }) + tc.wantIdle("connection is idle") } func TestConnInflowReturnOnRacingReads(t *testing.T) { @@ -63,11 +59,11 @@ func TestConnInflowReturnOnRacingReads(t *testing.T) { tc.ignoreFrame(frameTypeAck) tc.writeFrames(packetType1RTT, debugFrameStream{ id: newStreamID(clientSide, uniStream, 0), - data: make([]byte, 32), + data: make([]byte, 16), }) tc.writeFrames(packetType1RTT, debugFrameStream{ id: newStreamID(clientSide, uniStream, 1), - data: make([]byte, 32), + data: make([]byte, 1), }) s1, err := tc.conn.AcceptStream(ctx) if err != nil { @@ -203,7 +199,6 @@ func TestConnInflowResetViolation(t *testing.T) { } func TestConnInflowMultipleStreams(t *testing.T) { - ctx := canceledContext() tc := newTestConn(t, serverSide, func(c *Config) { c.MaxConnReadBufferSize = 128 }) @@ -219,12 +214,9 @@ func TestConnInflowMultipleStreams(t *testing.T) { } { tc.writeFrames(packetType1RTT, debugFrameStream{ id: id, - data: make([]byte, 32), + data: make([]byte, 1), }) - s, err := tc.conn.AcceptStream(ctx) - if err != nil { - t.Fatalf("AcceptStream() = %v", err) - } + s := tc.acceptStream() streams = append(streams, s) if n, err := s.Read(make([]byte, 1)); err != nil || n != 1 { t.Fatalf("s.Read() = %v, %v; want 1, nil", n, err) @@ -232,8 +224,16 @@ func TestConnInflowMultipleStreams(t *testing.T) { } tc.wantIdle("streams have read data, but not enough to update MAX_DATA") - if n, err := streams[0].Read(make([]byte, 32)); err != nil || n != 31 { - t.Fatalf("s.Read() = %v, %v; want 31, nil", n, err) + for _, s := range streams { + tc.writeFrames(packetType1RTT, debugFrameStream{ + id: s.id, + off: 1, + data: make([]byte, 31), + }) + } + + if n, err := streams[0].Read(make([]byte, 32)); n != 31 { + t.Fatalf("s.Read() = %v, %v; want 31, anything", n, err) } tc.wantFrame("read enough data to trigger a MAX_DATA update", packetType1RTT, debugFrameMaxData{ diff --git a/internal/quic/conn_loss_test.go b/internal/quic/conn_loss_test.go index 876ffd093..86ef23db0 100644 --- a/internal/quic/conn_loss_test.go +++ b/internal/quic/conn_loss_test.go @@ -308,9 +308,9 @@ func TestLostMaxDataFrame(t *testing.T) { tc.writeFrames(packetType1RTT, debugFrameStream{ id: s.id, off: 0, - data: make([]byte, maxWindowSize), + data: make([]byte, maxWindowSize-1), }) - if n, err := s.Read(buf[:maxWindowSize-1]); err != nil || n != maxWindowSize-1 { + if n, err := s.Read(buf[:maxWindowSize]); err != nil || n != maxWindowSize-1 { t.Fatalf("Read() = %v, %v; want %v, nil", n, err, maxWindowSize-1) } tc.wantFrame("conn window is extended after reading data", @@ -319,7 +319,12 @@ func TestLostMaxDataFrame(t *testing.T) { }) // MAX_DATA = 64, which is only one more byte, so we don't send the frame. - if n, err := s.Read(buf); err != nil || n != 1 { + tc.writeFrames(packetType1RTT, debugFrameStream{ + id: s.id, + off: maxWindowSize - 1, + data: make([]byte, 1), + }) + if n, err := s.Read(buf[:1]); err != nil || n != 1 { t.Fatalf("Read() = %v, %v; want %v, nil", n, err, 1) } tc.wantIdle("read doesn't extend window enough to send another MAX_DATA") @@ -348,9 +353,9 @@ func TestLostMaxStreamDataFrame(t *testing.T) { tc.writeFrames(packetType1RTT, debugFrameStream{ id: s.id, off: 0, - data: make([]byte, maxWindowSize), + data: make([]byte, maxWindowSize-1), }) - if n, err := s.Read(buf[:maxWindowSize-1]); err != nil || n != maxWindowSize-1 { + if n, err := s.Read(buf[:maxWindowSize]); err != nil || n != maxWindowSize-1 { t.Fatalf("Read() = %v, %v; want %v, nil", n, err, maxWindowSize-1) } tc.wantFrame("stream window is extended after reading data", @@ -360,6 +365,11 @@ func TestLostMaxStreamDataFrame(t *testing.T) { }) // MAX_STREAM_DATA = 64, which is only one more byte, so we don't send the frame. + tc.writeFrames(packetType1RTT, debugFrameStream{ + id: s.id, + off: maxWindowSize - 1, + data: make([]byte, 1), + }) if n, err := s.Read(buf); err != nil || n != 1 { t.Fatalf("Read() = %v, %v; want %v, nil", n, err, 1) } diff --git a/internal/quic/pipe.go b/internal/quic/pipe.go index d3a448df3..42a0049da 100644 --- a/internal/quic/pipe.go +++ b/internal/quic/pipe.go @@ -17,14 +17,14 @@ import ( // Writing past the end of the window extends it. // Data may be discarded from the start of the pipe, advancing the window. type pipe struct { - start int64 - end int64 - head *pipebuf - tail *pipebuf + start int64 // stream position of first stored byte + end int64 // stream position just past the last stored byte + head *pipebuf // if non-nil, then head.off + len(head.b) > start + tail *pipebuf // if non-nil, then tail.off + len(tail.b) == end } type pipebuf struct { - off int64 + off int64 // stream position of b[0] b []byte next *pipebuf } @@ -111,6 +111,7 @@ func (p *pipe) copy(off int64, b []byte) { // read calls f with the data in [off, off+n) // The data may be provided sequentially across multiple calls to f. +// Note that read (unlike an io.Reader) does not consume the read data. func (p *pipe) read(off int64, n int, f func([]byte) error) error { if off < p.start { panic("invalid read range") @@ -135,6 +136,18 @@ func (p *pipe) read(off int64, n int, f func([]byte) error) error { return nil } +// peek returns a reference to up to n bytes of internal data buffer, starting at p.start. +// The returned slice is valid until the next call to discardBefore. +// The length of the returned slice will be in the range [0,n]. +func (p *pipe) peek(n int64) []byte { + pb := p.head + if pb == nil { + return nil + } + b := pb.b[p.start-pb.off:] + return b[:min(int64(len(b)), n)] +} + // discardBefore discards all data prior to off. func (p *pipe) discardBefore(off int64) { for p.head != nil && p.head.end() < off { diff --git a/internal/quic/stream.go b/internal/quic/stream.go index 670b34263..17ca8b7d6 100644 --- a/internal/quic/stream.go +++ b/internal/quic/stream.go @@ -23,7 +23,7 @@ type Stream struct { inctx context.Context outctx context.Context - // ingate's lock guards all receive-related state. + // ingate's lock guards receive-related state. // // The gate condition is set if a read from the stream will not block, // either because the stream has available data or because the read will fail. @@ -37,7 +37,7 @@ type Stream struct { inclosed sentVal // set by CloseRead inresetcode int64 // RESET_STREAM code received from the peer; -1 if not reset - // outgate's lock guards all send-related state. + // outgate's lock guards send-related state. // // The gate condition is set if a write to the stream will not block, // either because the stream has available flow control or because @@ -57,6 +57,10 @@ type Stream struct { outresetcode uint64 // reset code to send in RESET_STREAM outdone chan struct{} // closed when all data sent + // Unsynchronized buffers, used for lock-free fast path. + inbuf []byte // received data + inbufoff int // bytes of inbuf which have been consumed + // Atomic stream state bits. // // These bits provide a fast way to coordinate between the @@ -202,16 +206,35 @@ func (s *Stream) IsWriteOnly() bool { // returning all data sent by the peer. // If the peer aborts reads on the stream, Read returns // an error wrapping StreamResetCode. +// +// It is not safe to call Read concurrently. func (s *Stream) Read(b []byte) (n int, err error) { if s.IsWriteOnly() { return 0, errors.New("read from write-only stream") } + if len(s.inbuf) > s.inbufoff { + // Fast path: If s.inbuf contains unread bytes, return them immediately + // without taking a lock. + n = copy(b, s.inbuf[s.inbufoff:]) + s.inbufoff += n + return n, nil + } if err := s.ingate.waitAndLock(s.inctx, s.conn.testHooks); err != nil { return 0, err } + if s.inbufoff > 0 { + // Discard bytes consumed by the fast path above. + s.in.discardBefore(s.in.start + int64(s.inbufoff)) + s.inbufoff = 0 + s.inbuf = nil + } + // bytesRead contains the number of bytes of connection-level flow control to return. + // We return flow control for bytes read by this Read call, as well as bytes moved + // to the fast-path read buffer (s.inbuf). + var bytesRead int64 defer func() { s.inUnlock() - s.conn.handleStreamBytesReadOffLoop(int64(n)) // must be done with ingate unlocked + s.conn.handleStreamBytesReadOffLoop(bytesRead) // must be done with ingate unlocked }() if s.inresetcode != -1 { return 0, fmt.Errorf("stream reset by peer: %w", StreamErrorCode(s.inresetcode)) @@ -229,27 +252,48 @@ func (s *Stream) Read(b []byte) (n int, err error) { if size := int(s.inset[0].end - s.in.start); size < len(b) { b = b[:size] } + bytesRead = int64(len(b)) start := s.in.start end := start + int64(len(b)) s.in.copy(start, b) s.in.discardBefore(end) + if end == s.insize { + // We have read up to the end of the stream. + // No need to update stream flow control. + return len(b), io.EOF + } + if len(s.inset) > 0 && s.inset[0].start <= s.in.start && s.inset[0].end > s.in.start { + // If we have more readable bytes available, put the next chunk of data + // in s.inbuf for lock-free reads. + s.inbuf = s.in.peek(s.inset[0].end - s.in.start) + bytesRead += int64(len(s.inbuf)) + } if s.insize == -1 || s.insize > s.inwin { - if shouldUpdateFlowControl(s.inmaxbuf, s.in.start+s.inmaxbuf-s.inwin) { + newWindow := s.in.start + int64(len(s.inbuf)) + s.inmaxbuf + addedWindow := newWindow - s.inwin + if shouldUpdateFlowControl(s.inmaxbuf, addedWindow) { // Update stream flow control with a STREAM_MAX_DATA frame. s.insendmax.setUnsent() } } - if end == s.insize { - return len(b), io.EOF - } return len(b), nil } // ReadByte reads and returns a single byte from the stream. +// +// It is not safe to call ReadByte concurrently. func (s *Stream) ReadByte() (byte, error) { + if len(s.inbuf) > s.inbufoff { + b := s.inbuf[s.inbufoff] + s.inbufoff++ + return b, nil + } var b [1]byte - _, err := s.Read(b[:]) - return b[0], err + n, err := s.Read(b[:]) + if n > 0 { + return b[0], err + } + return 0, err } // shouldUpdateFlowControl determines whether to send a flow control window update. @@ -507,8 +551,9 @@ func (s *Stream) inUnlock() { // inUnlockNoQueue is inUnlock, // but reports whether s has frames to write rather than notifying the Conn. func (s *Stream) inUnlockNoQueue() streamState { - canRead := s.inset.contains(s.in.start) || // data available to read - s.insize == s.in.start || // at EOF + nextByte := s.in.start + int64(len(s.inbuf)) + canRead := s.inset.contains(nextByte) || // data available to read + s.insize == s.in.start+int64(len(s.inbuf)) || // at EOF s.inresetcode != -1 || // reset by peer s.inclosed.isSet() // closed locally defer s.ingate.unlock(canRead) diff --git a/internal/quic/stream_test.go b/internal/quic/stream_test.go index 08e89b24c..d1cfb34db 100644 --- a/internal/quic/stream_test.go +++ b/internal/quic/stream_test.go @@ -538,6 +538,32 @@ func TestStreamReceiveDuplicateDataDoesNotViolateLimits(t *testing.T) { }) } +func TestStreamReceiveEmptyEOF(t *testing.T) { + // A stream receives some data, we read a byte of that data + // (causing the rest to be pulled into the s.inbuf buffer), + // and then we receive a FIN with no additional data. + testStreamTypes(t, "", func(t *testing.T, styp streamType) { + tc, s := newTestConnAndRemoteStream(t, serverSide, styp, permissiveTransportParameters) + want := []byte{1, 2, 3} + tc.writeFrames(packetType1RTT, debugFrameStream{ + id: s.id, + data: want, + }) + if got, err := s.ReadByte(); got != want[0] || err != nil { + t.Fatalf("s.ReadByte() = %v, %v; want %v, nil", got, err, want[0]) + } + + tc.writeFrames(packetType1RTT, debugFrameStream{ + id: s.id, + off: 3, + fin: true, + }) + if got, err := io.ReadAll(s); !bytes.Equal(got, want[1:]) || err != nil { + t.Fatalf("io.ReadAll(s) = {%x}, %v; want {%x}, nil", got, err, want[1:]) + } + }) +} + func finalSizeTest(t *testing.T, wantErr transportError, f func(tc *testConn, sid streamID) (finalSize int64), opts ...any) { testStreamTypes(t, "", func(t *testing.T, styp streamType) { for _, test := range []struct { @@ -1156,8 +1182,8 @@ func TestStreamPeerResetsWithUnreadAndUnsentData(t *testing.T) { code: sentCode, }) wantErr := StreamErrorCode(sentCode) - if n, err := s.Read(got); n != 0 || !errors.Is(err, wantErr) { - t.Fatalf("Read reset stream: got %v, %v; want 0, %v", n, err, wantErr) + if _, err := io.ReadAll(s); !errors.Is(err, wantErr) { + t.Fatalf("Read reset stream: ReadAll got error %v; want %v", err, wantErr) } }) } From 5e097125fdec6a2b4d9123a57f9551c2b89c7315 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 14 Feb 2024 12:31:31 -0800 Subject: [PATCH 10/22] quic: fast path for stream writes Similar to the fast-path for reads, writes are buffered in an unsynchronized []byte allowing for lock-free small writes. For golang/go#58547 Change-Id: I305cb5f91eff662a473f44a4bc051acc7c213e4c Reviewed-on: https://go-review.googlesource.com/c/net/+/564496 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- internal/quic/pipe.go | 12 +++++++++ internal/quic/stream.go | 50 ++++++++++++++++++++++++++++++++++-- internal/quic/stream_test.go | 3 ++- 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/internal/quic/pipe.go b/internal/quic/pipe.go index 42a0049da..75cf76db2 100644 --- a/internal/quic/pipe.go +++ b/internal/quic/pipe.go @@ -148,6 +148,18 @@ func (p *pipe) peek(n int64) []byte { return b[:min(int64(len(b)), n)] } +// availableBuffer returns the available contiguous, allocated buffer space +// following the pipe window. +// +// This is used by the stream write fast path, which makes multiple writes into the pipe buffer +// without a lock, and then adjusts p.end at a later time with a lock held. +func (p *pipe) availableBuffer() []byte { + if p.tail == nil { + return nil + } + return p.tail.b[p.end-p.tail.off:] +} + // discardBefore discards all data prior to off. func (p *pipe) discardBefore(off int64) { for p.head != nil && p.head.end() < off { diff --git a/internal/quic/stream.go b/internal/quic/stream.go index 17ca8b7d6..c5fafdf1d 100644 --- a/internal/quic/stream.go +++ b/internal/quic/stream.go @@ -58,8 +58,10 @@ type Stream struct { outdone chan struct{} // closed when all data sent // Unsynchronized buffers, used for lock-free fast path. - inbuf []byte // received data - inbufoff int // bytes of inbuf which have been consumed + inbuf []byte // received data + inbufoff int // bytes of inbuf which have been consumed + outbuf []byte // written data + outbufoff int // bytes of outbuf which contain data to write // Atomic stream state bits. // @@ -313,7 +315,14 @@ func (s *Stream) Write(b []byte) (n int, err error) { if s.IsReadOnly() { return 0, errors.New("write to read-only stream") } + if len(b) > 0 && len(s.outbuf)-s.outbufoff >= len(b) { + // Fast path: The data to write fits in s.outbuf. + copy(s.outbuf[s.outbufoff:], b) + s.outbufoff += len(b) + return len(b), nil + } canWrite := s.outgate.lock() + s.flushFastOutputBuffer() for { // The first time through this loop, we may or may not be write blocked. // We exit the loop after writing all data, so on subsequent passes through @@ -373,17 +382,51 @@ func (s *Stream) Write(b []byte) (n int, err error) { // If we have bytes left to send, we're blocked. canWrite = false } + if lim := s.out.start + s.outmaxbuf - s.out.end - 1; lim > 0 { + // If s.out has space allocated and available to be written into, + // then reference it in s.outbuf for fast-path writes. + // + // It's perhaps a bit pointless to limit s.outbuf to the send buffer limit. + // We've already allocated this buffer so we aren't saving any memory + // by not using it. + // For now, we limit it anyway to make it easier to reason about limits. + // + // We set the limit to one less than the send buffer limit (the -1 above) + // so that a write which completely fills the buffer will overflow + // s.outbuf and trigger a flush. + s.outbuf = s.out.availableBuffer() + if int64(len(s.outbuf)) > lim { + s.outbuf = s.outbuf[:lim] + } + } s.outUnlock() return n, nil } // WriteBytes writes a single byte to the stream. func (s *Stream) WriteByte(c byte) error { + if s.outbufoff < len(s.outbuf) { + s.outbuf[s.outbufoff] = c + s.outbufoff++ + return nil + } b := [1]byte{c} _, err := s.Write(b[:]) return err } +func (s *Stream) flushFastOutputBuffer() { + if s.outbuf == nil { + return + } + // Commit data previously written to s.outbuf. + // s.outbuf is a reference to a buffer in s.out, so we just need to record + // that the output buffer has been extended. + s.out.end += int64(s.outbufoff) + s.outbuf = nil + s.outbufoff = 0 +} + // Flush flushes data written to the stream. // It does not wait for the peer to acknowledge receipt of the data. // Use Close to wait for the peer's acknowledgement. @@ -394,6 +437,7 @@ func (s *Stream) Flush() { } func (s *Stream) flushLocked() { + s.flushFastOutputBuffer() s.outopened.set() if s.outflushed < s.outwin { s.outunsent.add(s.outflushed, min(s.outwin, s.out.end)) @@ -509,6 +553,8 @@ func (s *Stream) resetInternal(code uint64, userClosed bool) { // extra RESET_STREAM in this case is harmless. s.outreset.set() s.outresetcode = code + s.outbuf = nil + s.outbufoff = 0 s.out.discardBefore(s.out.end) s.outunsent = rangeset[int64]{} s.outblocked.clear() diff --git a/internal/quic/stream_test.go b/internal/quic/stream_test.go index d1cfb34db..9f857f29d 100644 --- a/internal/quic/stream_test.go +++ b/internal/quic/stream_test.go @@ -100,6 +100,7 @@ func TestStreamWriteBlockedByStreamFlowControl(t *testing.T) { if err != nil { t.Fatalf("write with available output buffer: unexpected error: %v", err) } + s.Flush() tc.wantFrame("write blocked by flow control triggers a STREAM_DATA_BLOCKED frame", packetType1RTT, debugFrameStreamDataBlocked{ id: s.id, @@ -111,6 +112,7 @@ func TestStreamWriteBlockedByStreamFlowControl(t *testing.T) { if err != nil { t.Fatalf("write with available output buffer: unexpected error: %v", err) } + s.Flush() tc.wantIdle("adding more blocked data does not trigger another STREAM_DATA_BLOCKED") // Provide some flow control window. @@ -1349,7 +1351,6 @@ func TestStreamFlushImplicitExact(t *testing.T) { id: s.id, data: want[0:4], }) - }) } From 2a8baeab1851a3c0f336a9185a02c177a0365232 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Sun, 18 Feb 2024 12:13:12 -0800 Subject: [PATCH 11/22] quic: don't record fin bit as sent when it wasn't When appendStreamFrame is provided with the last chunk of data for a stream, doesn't have enough space in the packet to include all the data, don't incorrectly record the packet as including a FIN bit. We were correctly sending a STREAM frame with no FIN bit--it's just the sent packet accounting that was off. No test, because I can't figure out a scenario where this actually has an observable effect, since we're always going to send the FIN when the remaining stream data is sent. Change-Id: I0ee81273165fcf10a52da76b33d2bf1b9c4f3523 Reviewed-on: https://go-review.googlesource.com/c/net/+/564796 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- internal/quic/packet_writer.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/quic/packet_writer.go b/internal/quic/packet_writer.go index 85149f607..9ed393502 100644 --- a/internal/quic/packet_writer.go +++ b/internal/quic/packet_writer.go @@ -388,11 +388,7 @@ func (w *packetWriter) appendStreamFrame(id streamID, off int64, size int, fin b w.b = appendVarint(w.b, uint64(size)) start := len(w.b) w.b = w.b[:start+size] - if fin { - w.sent.appendAckElicitingFrame(frameTypeStreamBase | streamFinBit) - } else { - w.sent.appendAckElicitingFrame(frameTypeStreamBase) - } + w.sent.appendAckElicitingFrame(typ & (frameTypeStreamBase | streamFinBit)) w.sent.appendInt(uint64(id)) w.sent.appendOffAndSize(off, size) return w.b[start:][:size], true From a6a24dd292f82221e069bd497ff2a93756f63d20 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 15 Feb 2024 09:52:29 -0800 Subject: [PATCH 12/22] quic: source address and ECN support in the network layer Make the abstraction over UDP connections higher level, and add support for setting the source address and ECN bits in sent packets, and receving the destination address and ECN bits in received packets. There is no good way that I can find to identify the source IP address of packets we send. Look up the destination IP address of the first packet received on each connection, and use this as the source address for all future packets we send. This avoids unexpected path migration, where the address we send from changes without our knowing it. Reject received packets sent from an unexpected peer address. In the future, when we support path migration, we will want to relax these restrictions. ECN bits may be used to detect network congestion. We don't make use of them at this time, but this CL adds the necessary UDP layer support to do so in the future. This CL also lays the groundwork for using more efficient platform APIs to send/receive packets in the future. (sendmmsg/recvmmsg/GSO/GRO) These features require platform-specific APIs. Add support for Darwin and Linux to start with, with a graceful fallback on other OSs. For golang/go#58547 Change-Id: I1c97cc0d3e52fff18e724feaaac4a50d3df671bc Reviewed-on: https://go-review.googlesource.com/c/net/+/565255 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/conn.go | 9 +- internal/quic/conn_recv.go | 38 ++-- internal/quic/conn_send.go | 5 +- internal/quic/conn_test.go | 7 + internal/quic/dgram.go | 23 ++- internal/quic/endpoint.go | 90 +++++----- internal/quic/endpoint_test.go | 26 ++- internal/quic/qlog.go | 6 + internal/quic/retry.go | 17 +- internal/quic/retry_test.go | 4 +- internal/quic/stateless_reset_test.go | 4 +- internal/quic/udp.go | 30 ++++ internal/quic/udp_darwin.go | 13 ++ internal/quic/udp_linux.go | 13 ++ internal/quic/udp_msg.go | 248 ++++++++++++++++++++++++++ internal/quic/udp_other.go | 62 +++++++ internal/quic/udp_test.go | 176 ++++++++++++++++++ 17 files changed, 676 insertions(+), 95 deletions(-) create mode 100644 internal/quic/udp.go create mode 100644 internal/quic/udp_darwin.go create mode 100644 internal/quic/udp_linux.go create mode 100644 internal/quic/udp_msg.go create mode 100644 internal/quic/udp_other.go create mode 100644 internal/quic/udp_test.go diff --git a/internal/quic/conn.go b/internal/quic/conn.go index 020bc81a4..5738b6dbb 100644 --- a/internal/quic/conn.go +++ b/internal/quic/conn.go @@ -25,6 +25,7 @@ type Conn struct { config *Config testHooks connTestHooks peerAddr netip.AddrPort + localAddr netip.AddrPort msgc chan any donec chan struct{} // closed when conn loop exits @@ -97,7 +98,7 @@ func newConn(now time.Time, side connSide, cids newServerConnIDs, peerAddr netip side: side, endpoint: e, config: config, - peerAddr: peerAddr, + peerAddr: unmapAddrPort(peerAddr), msgc: make(chan any, 1), donec: make(chan struct{}), peerAckDelayExponent: -1, @@ -317,7 +318,11 @@ func (c *Conn) loop(now time.Time) { } switch m := m.(type) { case *datagram: - c.handleDatagram(now, m) + if !c.handleDatagram(now, m) { + if c.logEnabled(QLogLevelPacket) { + c.logPacketDropped(m) + } + } m.recycle() case timerEvent: // A connection timer has expired. diff --git a/internal/quic/conn_recv.go b/internal/quic/conn_recv.go index 1b3219723..c8d70d85c 100644 --- a/internal/quic/conn_recv.go +++ b/internal/quic/conn_recv.go @@ -8,17 +8,33 @@ package quic import ( "bytes" - "context" "encoding/binary" "errors" "time" ) -func (c *Conn) handleDatagram(now time.Time, dgram *datagram) { +func (c *Conn) handleDatagram(now time.Time, dgram *datagram) (handled bool) { + if !c.localAddr.IsValid() { + // We don't have any way to tell in the general case what address we're + // sending packets from. Set our address from the destination address of + // the first packet received from the peer. + c.localAddr = dgram.localAddr + } + if dgram.peerAddr.IsValid() && dgram.peerAddr != c.peerAddr { + if c.side == clientSide { + // "If a client receives packets from an unknown server address, + // the client MUST discard these packets." + // https://www.rfc-editor.org/rfc/rfc9000#section-9-6 + return false + } + // We currently don't support connection migration, + // so for now the server also drops packets from an unknown address. + return false + } buf := dgram.b c.loss.datagramReceived(now, len(buf)) if c.isDraining() { - return + return false } for len(buf) > 0 { var n int @@ -28,7 +44,7 @@ func (c *Conn) handleDatagram(now time.Time, dgram *datagram) { if c.side == serverSide && len(dgram.b) < paddedInitialDatagramSize { // Discard client-sent Initial packets in too-short datagrams. // https://www.rfc-editor.org/rfc/rfc9000#section-14.1-4 - return + return false } n = c.handleLongHeader(now, ptype, initialSpace, c.keysInitial.r, buf) case packetTypeHandshake: @@ -37,10 +53,10 @@ func (c *Conn) handleDatagram(now time.Time, dgram *datagram) { n = c.handle1RTT(now, buf) case packetTypeRetry: c.handleRetry(now, buf) - return + return true case packetTypeVersionNegotiation: c.handleVersionNegotiation(now, buf) - return + return true default: n = -1 } @@ -58,20 +74,16 @@ func (c *Conn) handleDatagram(now time.Time, dgram *datagram) { var token statelessResetToken copy(token[:], buf[len(buf)-len(token):]) if c.handleStatelessReset(now, token) { - return + return true } } // Invalid data at the end of a datagram is ignored. - if c.logEnabled(QLogLevelPacket) { - c.log.LogAttrs(context.Background(), QLogLevelPacket, - "connectivity:packet_dropped", - ) - } - break + return false } c.idleHandlePacketReceived(now) buf = buf[n:] } + return true } func (c *Conn) handleLongHeader(now time.Time, ptype packetType, space numberSpace, k fixedKeys, buf []byte) int { diff --git a/internal/quic/conn_send.go b/internal/quic/conn_send.go index 575b8f9b4..12bcfe308 100644 --- a/internal/quic/conn_send.go +++ b/internal/quic/conn_send.go @@ -179,7 +179,10 @@ func (c *Conn) maybeSend(now time.Time) (next time.Time) { } } - c.endpoint.sendDatagram(buf, c.peerAddr) + c.endpoint.sendDatagram(datagram{ + b: buf, + peerAddr: c.peerAddr, + }) } } diff --git a/internal/quic/conn_test.go b/internal/quic/conn_test.go index 2d3c946d6..a8f3fc7fd 100644 --- a/internal/quic/conn_test.go +++ b/internal/quic/conn_test.go @@ -453,6 +453,7 @@ func (tc *testConn) writeFrames(ptype packetType, frames ...debugFrame) { dstConnID: dstConnID, srcConnID: tc.peerConnID, }}, + addr: tc.conn.peerAddr, } if ptype == packetTypeInitial && tc.conn.side == serverSide { d.paddedSize = 1200 @@ -656,6 +657,12 @@ func (tc *testConn) wantPacket(expectation string, want *testPacket) { } func packetEqual(a, b *testPacket) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } ac := *a ac.frames = nil ac.header = 0 diff --git a/internal/quic/dgram.go b/internal/quic/dgram.go index 79e6650fa..615589373 100644 --- a/internal/quic/dgram.go +++ b/internal/quic/dgram.go @@ -12,10 +12,25 @@ import ( ) type datagram struct { - b []byte - addr netip.AddrPort + b []byte + localAddr netip.AddrPort + peerAddr netip.AddrPort + ecn ecnBits } +// Explicit Congestion Notification bits. +// +// https://www.rfc-editor.org/rfc/rfc3168.html#section-5 +type ecnBits byte + +const ( + ecnMask = 0b000000_11 + ecnNotECT = 0b000000_00 + ecnECT1 = 0b000000_01 + ecnECT0 = 0b000000_10 + ecnCE = 0b000000_11 +) + var datagramPool = sync.Pool{ New: func() any { return &datagram{ @@ -26,7 +41,9 @@ var datagramPool = sync.Pool{ func newDatagram() *datagram { m := datagramPool.Get().(*datagram) - m.b = m.b[:cap(m.b)] + *m = datagram{ + b: m.b[:cap(m.b)], + } return m } diff --git a/internal/quic/endpoint.go b/internal/quic/endpoint.go index 8ed67de54..6631708b8 100644 --- a/internal/quic/endpoint.go +++ b/internal/quic/endpoint.go @@ -22,11 +22,11 @@ import ( // // Multiple goroutines may invoke methods on an Endpoint simultaneously. type Endpoint struct { - config *Config - udpConn udpConn - testHooks endpointTestHooks - resetGen statelessResetTokenGenerator - retry retryState + config *Config + packetConn packetConn + testHooks endpointTestHooks + resetGen statelessResetTokenGenerator + retry retryState acceptQueue queue[*Conn] // new inbound connections connsMap connsMap // only accessed by the listen loop @@ -42,13 +42,12 @@ type endpointTestHooks interface { newConn(c *Conn) } -// A udpConn is a UDP connection. -// It is implemented by net.UDPConn. -type udpConn interface { +// A packetConn is the interface to sending and receiving UDP packets. +type packetConn interface { Close() error - LocalAddr() net.Addr - ReadMsgUDPAddrPort(b, control []byte) (n, controln, flags int, _ netip.AddrPort, _ error) - WriteToUDPAddrPort(b []byte, addr netip.AddrPort) (int, error) + LocalAddr() netip.AddrPort + Read(f func(*datagram)) + Write(datagram) error } // Listen listens on a local network address. @@ -65,13 +64,17 @@ func Listen(network, address string, config *Config) (*Endpoint, error) { if err != nil { return nil, err } - return newEndpoint(udpConn, config, nil) + pc, err := newNetUDPConn(udpConn) + if err != nil { + return nil, err + } + return newEndpoint(pc, config, nil) } -func newEndpoint(udpConn udpConn, config *Config, hooks endpointTestHooks) (*Endpoint, error) { +func newEndpoint(pc packetConn, config *Config, hooks endpointTestHooks) (*Endpoint, error) { e := &Endpoint{ config: config, - udpConn: udpConn, + packetConn: pc, testHooks: hooks, conns: make(map[*Conn]struct{}), acceptQueue: newQueue[*Conn](), @@ -90,8 +93,7 @@ func newEndpoint(udpConn udpConn, config *Config, hooks endpointTestHooks) (*End // LocalAddr returns the local network address. func (e *Endpoint) LocalAddr() netip.AddrPort { - a, _ := e.udpConn.LocalAddr().(*net.UDPAddr) - return a.AddrPort() + return e.packetConn.LocalAddr() } // Close closes the Endpoint. @@ -114,7 +116,7 @@ func (e *Endpoint) Close(ctx context.Context) error { conns = append(conns, c) } if len(e.conns) == 0 { - e.udpConn.Close() + e.packetConn.Close() } } e.connsMu.Unlock() @@ -200,34 +202,18 @@ func (e *Endpoint) connDrained(c *Conn) { defer e.connsMu.Unlock() delete(e.conns, c) if e.closing && len(e.conns) == 0 { - e.udpConn.Close() + e.packetConn.Close() } } func (e *Endpoint) listen() { defer close(e.closec) - for { - m := newDatagram() - // TODO: Read and process the ECN (explicit congestion notification) field. - // https://tools.ietf.org/html/draft-ietf-quic-transport-32#section-13.4 - n, _, _, addr, err := e.udpConn.ReadMsgUDPAddrPort(m.b, nil) - if err != nil { - // The user has probably closed the endpoint. - // We currently don't surface errors from other causes; - // we could check to see if the endpoint has been closed and - // record the unexpected error if it has not. - return - } - if n == 0 { - continue - } + e.packetConn.Read(func(m *datagram) { if e.connsMap.updateNeeded.Load() { e.connsMap.applyUpdates() } - m.addr = addr - m.b = m.b[:n] e.handleDatagram(m) - } + }) } func (e *Endpoint) handleDatagram(m *datagram) { @@ -277,7 +263,7 @@ func (e *Endpoint) handleUnknownDestinationDatagram(m *datagram) { // If this is a 1-RTT packet, there's nothing productive we can do with it. // Send a stateless reset if possible. if !isLongHeader(m.b[0]) { - e.maybeSendStatelessReset(m.b, m.addr) + e.maybeSendStatelessReset(m.b, m.peerAddr) return } p, ok := parseGenericLongHeaderPacket(m.b) @@ -291,7 +277,7 @@ func (e *Endpoint) handleUnknownDestinationDatagram(m *datagram) { return default: // Unknown version. - e.sendVersionNegotiation(p, m.addr) + e.sendVersionNegotiation(p, m.peerAddr) return } if getPacketType(m.b) != packetTypeInitial { @@ -309,7 +295,7 @@ func (e *Endpoint) handleUnknownDestinationDatagram(m *datagram) { if e.config.RequireAddressValidation { var ok bool cids.retrySrcConnID = p.dstConnID - cids.originalDstConnID, ok = e.validateInitialAddress(now, p, m.addr) + cids.originalDstConnID, ok = e.validateInitialAddress(now, p, m.peerAddr) if !ok { return } @@ -317,7 +303,7 @@ func (e *Endpoint) handleUnknownDestinationDatagram(m *datagram) { cids.originalDstConnID = p.dstConnID } var err error - c, err := e.newConn(now, serverSide, cids, m.addr) + c, err := e.newConn(now, serverSide, cids, m.peerAddr) if err != nil { // The accept queue is probably full. // We could send a CONNECTION_CLOSE to the peer to reject the connection. @@ -329,7 +315,7 @@ func (e *Endpoint) handleUnknownDestinationDatagram(m *datagram) { m = nil // don't recycle, sendMsg takes ownership } -func (e *Endpoint) maybeSendStatelessReset(b []byte, addr netip.AddrPort) { +func (e *Endpoint) maybeSendStatelessReset(b []byte, peerAddr netip.AddrPort) { if !e.resetGen.canReset { // Config.StatelessResetKey isn't set, so we don't send stateless resets. return @@ -370,17 +356,21 @@ func (e *Endpoint) maybeSendStatelessReset(b []byte, addr netip.AddrPort) { b[0] &^= headerFormLong // clear long header bit b[0] |= fixedBit // set fixed bit copy(b[len(b)-statelessResetTokenLen:], token[:]) - e.sendDatagram(b, addr) + e.sendDatagram(datagram{ + b: b, + peerAddr: peerAddr, + }) } -func (e *Endpoint) sendVersionNegotiation(p genericLongPacket, addr netip.AddrPort) { +func (e *Endpoint) sendVersionNegotiation(p genericLongPacket, peerAddr netip.AddrPort) { m := newDatagram() m.b = appendVersionNegotiation(m.b[:0], p.srcConnID, p.dstConnID, quicVersion1) - e.sendDatagram(m.b, addr) + m.peerAddr = peerAddr + e.sendDatagram(*m) m.recycle() } -func (e *Endpoint) sendConnectionClose(in genericLongPacket, addr netip.AddrPort, code transportError) { +func (e *Endpoint) sendConnectionClose(in genericLongPacket, peerAddr netip.AddrPort, code transportError) { keys := initialKeys(in.dstConnID, serverSide) var w packetWriter p := longPacket{ @@ -399,12 +389,14 @@ func (e *Endpoint) sendConnectionClose(in genericLongPacket, addr netip.AddrPort if len(buf) == 0 { return } - e.sendDatagram(buf, addr) + e.sendDatagram(datagram{ + b: buf, + peerAddr: peerAddr, + }) } -func (e *Endpoint) sendDatagram(p []byte, addr netip.AddrPort) error { - _, err := e.udpConn.WriteToUDPAddrPort(p, addr) - return err +func (e *Endpoint) sendDatagram(dgram datagram) error { + return e.packetConn.Write(dgram) } // A connsMap is an endpoint's mapping of conn ids and reset tokens to conns. diff --git a/internal/quic/endpoint_test.go b/internal/quic/endpoint_test.go index 6d103f061..b9fb55fb3 100644 --- a/internal/quic/endpoint_test.go +++ b/internal/quic/endpoint_test.go @@ -12,7 +12,6 @@ import ( "crypto/tls" "io" "log/slog" - "net" "net/netip" "testing" "time" @@ -190,13 +189,9 @@ func (te *testEndpoint) writeDatagram(d *testDatagram) { for len(buf) < d.paddedSize { buf = append(buf, 0) } - addr := d.addr - if !addr.IsValid() { - addr = testClientAddr - } te.write(&datagram{ - b: buf, - addr: addr, + b: buf, + peerAddr: d.addr, }) } @@ -303,25 +298,24 @@ func (te *testEndpointUDPConn) Close() error { return nil } -func (te *testEndpointUDPConn) LocalAddr() net.Addr { - return net.UDPAddrFromAddrPort(netip.MustParseAddrPort("127.0.0.1:443")) +func (te *testEndpointUDPConn) LocalAddr() netip.AddrPort { + return netip.MustParseAddrPort("127.0.0.1:443") } -func (te *testEndpointUDPConn) ReadMsgUDPAddrPort(b, control []byte) (n, controln, flags int, _ netip.AddrPort, _ error) { +func (te *testEndpointUDPConn) Read(f func(*datagram)) { for { select { case d, ok := <-te.recvc: if !ok { - return 0, 0, 0, netip.AddrPort{}, io.EOF + return } - n = copy(b, d.b) - return n, 0, 0, d.addr, nil + f(d) case <-te.idlec: } } } -func (te *testEndpointUDPConn) WriteToUDPAddrPort(b []byte, addr netip.AddrPort) (int, error) { - te.sentDatagrams = append(te.sentDatagrams, append([]byte(nil), b...)) - return len(b), nil +func (te *testEndpointUDPConn) Write(dgram datagram) error { + te.sentDatagrams = append(te.sentDatagrams, append([]byte(nil), dgram.b...)) + return nil } diff --git a/internal/quic/qlog.go b/internal/quic/qlog.go index e37e2f8ce..36831252c 100644 --- a/internal/quic/qlog.go +++ b/internal/quic/qlog.go @@ -151,6 +151,12 @@ func (c *Conn) logConnectionClosed() { ) } +func (c *Conn) logPacketDropped(dgram *datagram) { + c.log.LogAttrs(context.Background(), QLogLevelPacket, + "connectivity:packet_dropped", + ) +} + func (c *Conn) logLongPacketReceived(p longPacket, pkt []byte) { var frames slog.Attr if c.logEnabled(QLogLevelFrame) { diff --git a/internal/quic/retry.go b/internal/quic/retry.go index 31cb57b88..5dc39d1d9 100644 --- a/internal/quic/retry.go +++ b/internal/quic/retry.go @@ -139,7 +139,7 @@ func (rs *retryState) additionalData(srcConnID []byte, addr netip.AddrPort) []by return additional } -func (e *Endpoint) validateInitialAddress(now time.Time, p genericLongPacket, addr netip.AddrPort) (origDstConnID []byte, ok bool) { +func (e *Endpoint) validateInitialAddress(now time.Time, p genericLongPacket, peerAddr netip.AddrPort) (origDstConnID []byte, ok bool) { // The retry token is at the start of an Initial packet's data. token, n := consumeUint8Bytes(p.data) if n < 0 { @@ -151,22 +151,22 @@ func (e *Endpoint) validateInitialAddress(now time.Time, p genericLongPacket, ad if len(token) == 0 { // The sender has not provided a token. // Send a Retry packet to them with one. - e.sendRetry(now, p, addr) + e.sendRetry(now, p, peerAddr) return nil, false } - origDstConnID, ok = e.retry.validateToken(now, token, p.srcConnID, p.dstConnID, addr) + origDstConnID, ok = e.retry.validateToken(now, token, p.srcConnID, p.dstConnID, peerAddr) if !ok { // This does not seem to be a valid token. // Close the connection with an INVALID_TOKEN error. // https://www.rfc-editor.org/rfc/rfc9000#section-8.1.2-5 - e.sendConnectionClose(p, addr, errInvalidToken) + e.sendConnectionClose(p, peerAddr, errInvalidToken) return nil, false } return origDstConnID, true } -func (e *Endpoint) sendRetry(now time.Time, p genericLongPacket, addr netip.AddrPort) { - token, srcConnID, err := e.retry.makeToken(now, p.srcConnID, p.dstConnID, addr) +func (e *Endpoint) sendRetry(now time.Time, p genericLongPacket, peerAddr netip.AddrPort) { + token, srcConnID, err := e.retry.makeToken(now, p.srcConnID, p.dstConnID, peerAddr) if err != nil { return } @@ -175,7 +175,10 @@ func (e *Endpoint) sendRetry(now time.Time, p genericLongPacket, addr netip.Addr srcConnID: srcConnID, token: token, }) - e.sendDatagram(b, addr) + e.sendDatagram(datagram{ + b: b, + peerAddr: peerAddr, + }) } type retryPacket struct { diff --git a/internal/quic/retry_test.go b/internal/quic/retry_test.go index 8f36e1bd3..42f2bdd4a 100644 --- a/internal/quic/retry_test.go +++ b/internal/quic/retry_test.go @@ -436,8 +436,8 @@ func TestRetryClientIgnoresRetryWithInvalidIntegrityTag(t *testing.T) { }) pkt[len(pkt)-1] ^= 1 // invalidate the integrity tag tc.endpoint.write(&datagram{ - b: pkt, - addr: testClientAddr, + b: pkt, + peerAddr: testClientAddr, }) tc.wantIdle("client ignores Retry with invalid integrity tag") } diff --git a/internal/quic/stateless_reset_test.go b/internal/quic/stateless_reset_test.go index 45a49e81e..9458d2ea9 100644 --- a/internal/quic/stateless_reset_test.go +++ b/internal/quic/stateless_reset_test.go @@ -57,8 +57,8 @@ func newDatagramForReset(cid []byte, size int, addr netip.AddrPort) *datagram { dgram = append(dgram, byte(len(dgram))) // semi-random junk } return &datagram{ - b: dgram, - addr: addr, + b: dgram, + peerAddr: addr, } } diff --git a/internal/quic/udp.go b/internal/quic/udp.go new file mode 100644 index 000000000..0a578286b --- /dev/null +++ b/internal/quic/udp.go @@ -0,0 +1,30 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 + +package quic + +import "net/netip" + +// Per-plaform consts describing support for various features. +// +// const udpECNSupport indicates whether the platform supports setting +// the ECN (Explicit Congestion Notification) IP header bits. +// +// const udpInvalidLocalAddrIsError indicates whether sending a packet +// from an local address not associated with the system is an error. +// For example, assuming 127.0.0.2 is not a local address, does sending +// from it (using IP_PKTINFO or some other such feature) result in an error? + +// unmapAddrPort returns a with any IPv4-mapped IPv6 address prefix removed. +func unmapAddrPort(a netip.AddrPort) netip.AddrPort { + if a.Addr().Is4In6() { + return netip.AddrPortFrom( + a.Addr().Unmap(), + a.Port(), + ) + } + return a +} diff --git a/internal/quic/udp_darwin.go b/internal/quic/udp_darwin.go new file mode 100644 index 000000000..3868a36a8 --- /dev/null +++ b/internal/quic/udp_darwin.go @@ -0,0 +1,13 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 && darwin + +package quic + +// See udp.go. +const ( + udpECNSupport = true + udpInvalidLocalAddrIsError = true +) diff --git a/internal/quic/udp_linux.go b/internal/quic/udp_linux.go new file mode 100644 index 000000000..2ba3e6f2f --- /dev/null +++ b/internal/quic/udp_linux.go @@ -0,0 +1,13 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 && linux + +package quic + +// See udp.go. +const ( + udpECNSupport = true + udpInvalidLocalAddrIsError = false +) diff --git a/internal/quic/udp_msg.go b/internal/quic/udp_msg.go new file mode 100644 index 000000000..bdc1b710d --- /dev/null +++ b/internal/quic/udp_msg.go @@ -0,0 +1,248 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 && !quicbasicnet && (darwin || linux) + +package quic + +import ( + "net" + "net/netip" + "sync" + "unsafe" + + "golang.org/x/sys/unix" +) + +// Network interface for platforms using sendmsg/recvmsg with cmsgs. + +type netUDPConn struct { + c *net.UDPConn + localAddr netip.AddrPort +} + +func newNetUDPConn(uc *net.UDPConn) (*netUDPConn, error) { + a, _ := uc.LocalAddr().(*net.UDPAddr) + localAddr := a.AddrPort() + if localAddr.Addr().IsUnspecified() { + // If the conn is not bound to a specified (non-wildcard) address, + // then set localAddr.Addr to an invalid netip.Addr. + // This better conveys that this is not an address we should be using, + // and is a bit more efficient to test against. + localAddr = netip.AddrPortFrom(netip.Addr{}, localAddr.Port()) + } + + sc, err := uc.SyscallConn() + if err != nil { + return nil, err + } + sc.Control(func(fd uintptr) { + // Ask for ECN info and (when we aren't bound to a fixed local address) + // destination info. + // + // If any of these calls fail, we won't get the requested information. + // That's fine, we'll gracefully handle the lack. + unix.SetsockoptInt(int(fd), unix.IPPROTO_IP, unix.IP_RECVTOS, 1) + unix.SetsockoptInt(int(fd), unix.IPPROTO_IPV6, unix.IPV6_RECVTCLASS, 1) + if !localAddr.IsValid() { + unix.SetsockoptInt(int(fd), unix.IPPROTO_IP, unix.IP_PKTINFO, 1) + unix.SetsockoptInt(int(fd), unix.IPPROTO_IPV6, unix.IPV6_RECVPKTINFO, 1) + } + }) + + return &netUDPConn{ + c: uc, + localAddr: localAddr, + }, nil +} + +func (c *netUDPConn) Close() error { return c.c.Close() } + +func (c *netUDPConn) LocalAddr() netip.AddrPort { + a, _ := c.c.LocalAddr().(*net.UDPAddr) + return a.AddrPort() +} + +func (c *netUDPConn) Read(f func(*datagram)) { + // We shouldn't ever see all of these messages at the same time, + // but the total is small so just allocate enough space for everything we use. + const ( + inPktinfoSize = 12 // int + in_addr + in_addr + in6PktinfoSize = 20 // in6_addr + int + ipTOSSize = 4 + ipv6TclassSize = 4 + ) + control := make([]byte, 0+ + unix.CmsgSpace(inPktinfoSize)+ + unix.CmsgSpace(in6PktinfoSize)+ + unix.CmsgSpace(ipTOSSize)+ + unix.CmsgSpace(ipv6TclassSize)) + + for { + d := newDatagram() + n, controlLen, _, peerAddr, err := c.c.ReadMsgUDPAddrPort(d.b, control) + if err != nil { + return + } + if n == 0 { + continue + } + d.localAddr = c.localAddr + d.peerAddr = unmapAddrPort(peerAddr) + d.b = d.b[:n] + parseControl(d, control[:controlLen]) + f(d) + } +} + +var cmsgPool = sync.Pool{ + New: func() any { + return new([]byte) + }, +} + +func (c *netUDPConn) Write(dgram datagram) error { + controlp := cmsgPool.Get().(*[]byte) + control := *controlp + defer func() { + *controlp = control[:0] + cmsgPool.Put(controlp) + }() + + localIP := dgram.localAddr.Addr() + if localIP.IsValid() { + if localIP.Is4() { + control = appendCmsgIPSourceAddrV4(control, localIP) + } else { + control = appendCmsgIPSourceAddrV6(control, localIP) + } + } + if dgram.ecn != ecnNotECT { + if dgram.peerAddr.Addr().Is4() { + control = appendCmsgECNv4(control, dgram.ecn) + } else { + control = appendCmsgECNv6(control, dgram.ecn) + } + } + + _, _, err := c.c.WriteMsgUDPAddrPort(dgram.b, control, dgram.peerAddr) + return err +} + +func parseControl(d *datagram, control []byte) { + for len(control) > 0 { + hdr, data, remainder, err := unix.ParseOneSocketControlMessage(control) + if err != nil { + return + } + control = remainder + switch hdr.Level { + case unix.IPPROTO_IP: + switch hdr.Type { + case unix.IP_TOS, unix.IP_RECVTOS: + // Single byte containing the IP TOS field. + // The low two bits are the ECN field. + // + // (Linux sets the type to IP_TOS, Darwin to IP_RECVTOS, + // jus check for both.) + if len(data) < 1 { + break + } + d.ecn = ecnBits(data[0] & ecnMask) + case unix.IP_PKTINFO: + if a, ok := parseInPktinfo(data); ok { + d.localAddr = netip.AddrPortFrom(a, d.localAddr.Port()) + } + } + case unix.IPPROTO_IPV6: + switch hdr.Type { + case unix.IPV6_TCLASS: + // Single byte containing the traffic class field. + // The low two bits are the ECN field. + if len(data) < 1 { + break + } + d.ecn = ecnBits(data[0] & ecnMask) + case unix.IPV6_PKTINFO: + if a, ok := parseIn6Pktinfo(data); ok { + d.localAddr = netip.AddrPortFrom(a, d.localAddr.Port()) + } + } + } + } +} + +func parseInPktinfo(b []byte) (netip.Addr, bool) { + // struct in_pktinfo { + // unsigned int ipi_ifindex; /* send/recv interface index */ + // struct in_addr ipi_spec_dst; /* Local address */ + // struct in_addr ipi_addr; /* IP Header dst address */ + // }; + if len(b) != 12 { + return netip.Addr{}, false + } + return netip.AddrFrom4([4]byte(b[8:][:4])), true +} + +func parseIn6Pktinfo(b []byte) (netip.Addr, bool) { + // struct in6_pktinfo { + // struct in6_addr ipi6_addr; /* src/dst IPv6 address */ + // unsigned int ipi6_ifindex; /* send/recv interface index */ + // }; + if len(b) != 20 { + return netip.Addr{}, false + } + return netip.AddrFrom16([16]byte(b[:16])).Unmap(), true +} + +// appendCmsgIPSourceAddrV4 appends an IP_PKTINFO setting the source address +// for an outbound datagram. +func appendCmsgIPSourceAddrV4(b []byte, src netip.Addr) []byte { + // struct in_pktinfo { + // unsigned int ipi_ifindex; /* send/recv interface index */ + // struct in_addr ipi_spec_dst; /* Local address */ + // struct in_addr ipi_addr; /* IP Header dst address */ + // }; + b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_PKTINFO, 12) + ip := src.As4() + copy(data[4:], ip[:]) + return b +} + +// appendCmsgIPSourceAddrV6 appends an IP_PKTINFO or IPV6_PKTINFO +// setting the source address for an outbound datagram. +func appendCmsgIPSourceAddrV6(b []byte, src netip.Addr) []byte { + // struct in6_pktinfo { + // struct in6_addr ipi6_addr; /* src/dst IPv6 address */ + // unsigned int ipi6_ifindex; /* send/recv interface index */ + // }; + b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_PKTINFO, 20) + ip := src.As16() + copy(data[0:], ip[:]) + return b +} + +func appendCmsgECNv4(b []byte, ecn ecnBits) []byte { + b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_TOS, 4) + data[0] = byte(ecn) + return b +} + +func appendCmsgECNv6(b []byte, ecn ecnBits) []byte { + b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_TCLASS, 4) + data[0] = byte(ecn) + return b +} + +// appendCmsg appends a cmsg with the given level, type, and size to b. +// It returns the new buffer, and the data section of the cmsg. +func appendCmsg(b []byte, level, typ int32, size int) (_, data []byte) { + off := len(b) + b = append(b, make([]byte, unix.CmsgSpace(size))...) + h := (*unix.Cmsghdr)(unsafe.Pointer(&b[off])) + h.Level = level + h.Type = typ + h.SetLen(unix.CmsgLen(size)) + return b, b[off+unix.CmsgSpace(0):][:size] +} diff --git a/internal/quic/udp_other.go b/internal/quic/udp_other.go new file mode 100644 index 000000000..28be6d200 --- /dev/null +++ b/internal/quic/udp_other.go @@ -0,0 +1,62 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 && (quicbasicnet || !(darwin || linux)) + +package quic + +import ( + "net" + "net/netip" +) + +// Lowest common denominator network interface: Basic net.UDPConn, no cmsgs. +// We will not be able to send or receive ECN bits, +// and we will not know what our local address is. +// +// The quicbasicnet build tag allows selecting this interface on any platform. + +// See udp.go. +const ( + udpECNSupport = false + udpInvalidLocalAddrIsError = false +) + +type netUDPConn struct { + c *net.UDPConn +} + +func newNetUDPConn(uc *net.UDPConn) (*netUDPConn, error) { + return &netUDPConn{ + c: uc, + }, nil +} + +func (c *netUDPConn) Close() error { return c.c.Close() } + +func (c *netUDPConn) LocalAddr() netip.AddrPort { + a, _ := c.c.LocalAddr().(*net.UDPAddr) + return a.AddrPort() +} + +func (c *netUDPConn) Read(f func(*datagram)) { + for { + dgram := newDatagram() + n, _, _, peerAddr, err := c.c.ReadMsgUDPAddrPort(dgram.b, nil) + if err != nil { + return + } + if n == 0 { + continue + } + dgram.peerAddr = unmapAddrPort(peerAddr) + dgram.b = dgram.b[:n] + f(dgram) + } +} + +func (c *netUDPConn) Write(dgram datagram) error { + _, err := c.c.WriteToUDPAddrPort(dgram.b, dgram.peerAddr) + return err +} diff --git a/internal/quic/udp_test.go b/internal/quic/udp_test.go new file mode 100644 index 000000000..27eddf811 --- /dev/null +++ b/internal/quic/udp_test.go @@ -0,0 +1,176 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 + +package quic + +import ( + "bytes" + "fmt" + "net" + "net/netip" + "runtime" + "testing" +) + +func TestUDPSourceUnspecified(t *testing.T) { + // Send datagram with no source address set. + runUDPTest(t, func(t *testing.T, test udpTest) { + data := []byte("source unspecified") + if err := test.src.Write(datagram{ + b: data, + peerAddr: test.dstAddr, + }); err != nil { + t.Fatalf("Write: %v", err) + } + got := <-test.dgramc + if !bytes.Equal(got.b, data) { + t.Errorf("got datagram {%x}, want {%x}", got.b, data) + } + }) +} + +func TestUDPSourceSpecified(t *testing.T) { + // Send datagram with source address set. + runUDPTest(t, func(t *testing.T, test udpTest) { + data := []byte("source specified") + if err := test.src.Write(datagram{ + b: data, + peerAddr: test.dstAddr, + localAddr: test.src.LocalAddr(), + }); err != nil { + t.Fatalf("Write: %v", err) + } + got := <-test.dgramc + if !bytes.Equal(got.b, data) { + t.Errorf("got datagram {%x}, want {%x}", got.b, data) + } + }) +} + +func TestUDPSourceInvalid(t *testing.T) { + // Send datagram with source address set to an address not associated with the connection. + if !udpInvalidLocalAddrIsError { + t.Skipf("%v: sending from invalid source succeeds", runtime.GOOS) + } + runUDPTest(t, func(t *testing.T, test udpTest) { + var localAddr netip.AddrPort + if test.src.LocalAddr().Addr().Is4() { + localAddr = netip.MustParseAddrPort("127.0.0.2:1234") + } else { + localAddr = netip.MustParseAddrPort("[::2]:1234") + } + data := []byte("source invalid") + if err := test.src.Write(datagram{ + b: data, + peerAddr: test.dstAddr, + localAddr: localAddr, + }); err == nil { + t.Errorf("Write with invalid localAddr succeeded; want error") + } + }) +} + +func TestUDPECN(t *testing.T) { + if !udpECNSupport { + t.Skipf("%v: no ECN support", runtime.GOOS) + } + // Send datagrams with ECN bits set, verify the ECN bits are received. + runUDPTest(t, func(t *testing.T, test udpTest) { + for _, ecn := range []ecnBits{ecnNotECT, ecnECT1, ecnECT0, ecnCE} { + if err := test.src.Write(datagram{ + b: []byte{1, 2, 3, 4}, + peerAddr: test.dstAddr, + ecn: ecn, + }); err != nil { + t.Fatalf("Write: %v", err) + } + got := <-test.dgramc + if got.ecn != ecn { + t.Errorf("sending ECN bits %x, got %x", ecn, got.ecn) + } + } + }) +} + +type udpTest struct { + src *netUDPConn + dst *netUDPConn + dstAddr netip.AddrPort + dgramc chan *datagram +} + +// runUDPTest calls f with a pair of UDPConns in a matrix of network variations: +// udp, udp4, and udp6, and variations on binding to an unspecified address (0.0.0.0) +// or a specified one. +func runUDPTest(t *testing.T, f func(t *testing.T, u udpTest)) { + for _, test := range []struct { + srcNet, srcAddr, dstNet, dstAddr string + }{ + {"udp4", "127.0.0.1", "udp", ""}, + {"udp4", "127.0.0.1", "udp4", ""}, + {"udp4", "127.0.0.1", "udp4", "127.0.0.1"}, + {"udp6", "::1", "udp", ""}, + {"udp6", "::1", "udp6", ""}, + {"udp6", "::1", "udp6", "::1"}, + } { + spec := "spec" + if test.dstAddr == "" { + spec = "unspec" + } + t.Run(fmt.Sprintf("%v/%v/%v", test.srcNet, test.dstNet, spec), func(t *testing.T) { + srcAddr := netip.AddrPortFrom(netip.MustParseAddr(test.srcAddr), 0) + srcConn, err := net.ListenUDP(test.srcNet, net.UDPAddrFromAddrPort(srcAddr)) + if err != nil { + // If ListenUDP fails here, we presumably don't have + // IPv4/IPv6 configured. + t.Skipf("ListenUDP(%q, %v) = %v", test.srcNet, srcAddr, err) + } + t.Cleanup(func() { srcConn.Close() }) + src, err := newNetUDPConn(srcConn) + if err != nil { + t.Fatalf("newNetUDPConn: %v", err) + } + + var dstAddr netip.AddrPort + if test.dstAddr != "" { + dstAddr = netip.AddrPortFrom(netip.MustParseAddr(test.dstAddr), 0) + } + dstConn, err := net.ListenUDP(test.dstNet, net.UDPAddrFromAddrPort(dstAddr)) + if err != nil { + t.Skipf("ListenUDP(%q, nil) = %v", test.dstNet, err) + } + dst, err := newNetUDPConn(dstConn) + if err != nil { + dstConn.Close() + t.Fatalf("newNetUDPConn: %v", err) + } + + dgramc := make(chan *datagram) + go func() { + defer close(dgramc) + dst.Read(func(dgram *datagram) { + dgramc <- dgram + }) + }() + t.Cleanup(func() { + dstConn.Close() + for range dgramc { + t.Errorf("test read unexpected datagram") + } + }) + + f(t, udpTest{ + src: src, + dst: dst, + dstAddr: netip.AddrPortFrom( + srcAddr.Addr(), + dst.LocalAddr().Port(), + ), + dgramc: dgramc, + }) + }) + } +} From 57e4cc7d885a72ee5111b227ba5790ea5a170656 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 28 Nov 2023 15:31:58 -0800 Subject: [PATCH 13/22] quic: handle PATH_CHALLENGE and PATH_RESPONSE frames We do not support path migration yet, and will ignore packets sent from anything other than the peer's original address. Handle PATH_CHALLENGE frames by sending a PATH_RESPONSE. Handle PATH_RESPONSE frames by closing the connection (since we never send a challenge to respond to). For golang/go#58547 Change-Id: I828b9dcb23e17f5edf3d605b8f04efdafb392807 Reviewed-on: https://go-review.googlesource.com/c/net/+/565795 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/conn.go | 1 + internal/quic/conn_loss_test.go | 23 ++++++++ internal/quic/conn_recv.go | 44 ++++++++++++--- internal/quic/conn_send.go | 7 +++ internal/quic/conn_test.go | 2 + internal/quic/frame_debug.go | 17 ++++-- internal/quic/packet_codec_test.go | 4 +- internal/quic/packet_parser.go | 11 ++-- internal/quic/packet_writer.go | 17 +++--- internal/quic/path.go | 89 ++++++++++++++++++++++++++++++ internal/quic/path_test.go | 66 ++++++++++++++++++++++ internal/quic/sent_packet.go | 6 ++ 12 files changed, 255 insertions(+), 32 deletions(-) create mode 100644 internal/quic/path.go create mode 100644 internal/quic/path_test.go diff --git a/internal/quic/conn.go b/internal/quic/conn.go index 5738b6dbb..d462e9617 100644 --- a/internal/quic/conn.go +++ b/internal/quic/conn.go @@ -37,6 +37,7 @@ type Conn struct { connIDState connIDState loss lossState streams streamsState + path pathState // Packet protection keys, CRYPTO streams, and TLS state. keysInitial fixedKeyPair diff --git a/internal/quic/conn_loss_test.go b/internal/quic/conn_loss_test.go index 86ef23db0..81d537803 100644 --- a/internal/quic/conn_loss_test.go +++ b/internal/quic/conn_loss_test.go @@ -663,6 +663,29 @@ func TestLostRetireConnectionIDFrame(t *testing.T) { }) } +func TestLostPathResponseFrame(t *testing.T) { + // "Responses to path validation using PATH_RESPONSE frames are sent just once." + // https://www.rfc-editor.org/rfc/rfc9000.html#section-13.3-3.12 + lostFrameTest(t, func(t *testing.T, pto bool) { + tc := newTestConn(t, clientSide) + tc.handshake() + tc.ignoreFrame(frameTypeAck) + tc.ignoreFrame(frameTypePing) + + data := pathChallengeData{0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef} + tc.writeFrames(packetType1RTT, debugFramePathChallenge{ + data: data, + }) + tc.wantFrame("response to PATH_CHALLENGE", + packetType1RTT, debugFramePathResponse{ + data: data, + }) + + tc.triggerLossOrPTO(packetType1RTT, pto) + tc.wantIdle("lost PATH_RESPONSE frame is not retransmitted") + }) +} + func TestLostHandshakeDoneFrame(t *testing.T) { // "The HANDSHAKE_DONE frame MUST be retransmitted until it is acknowledged." // https://www.rfc-editor.org/rfc/rfc9000.html#section-13.3-3.16 diff --git a/internal/quic/conn_recv.go b/internal/quic/conn_recv.go index c8d70d85c..b1354cd3a 100644 --- a/internal/quic/conn_recv.go +++ b/internal/quic/conn_recv.go @@ -46,11 +46,11 @@ func (c *Conn) handleDatagram(now time.Time, dgram *datagram) (handled bool) { // https://www.rfc-editor.org/rfc/rfc9000#section-14.1-4 return false } - n = c.handleLongHeader(now, ptype, initialSpace, c.keysInitial.r, buf) + n = c.handleLongHeader(now, dgram, ptype, initialSpace, c.keysInitial.r, buf) case packetTypeHandshake: - n = c.handleLongHeader(now, ptype, handshakeSpace, c.keysHandshake.r, buf) + n = c.handleLongHeader(now, dgram, ptype, handshakeSpace, c.keysHandshake.r, buf) case packetType1RTT: - n = c.handle1RTT(now, buf) + n = c.handle1RTT(now, dgram, buf) case packetTypeRetry: c.handleRetry(now, buf) return true @@ -86,7 +86,7 @@ func (c *Conn) handleDatagram(now time.Time, dgram *datagram) (handled bool) { return true } -func (c *Conn) handleLongHeader(now time.Time, ptype packetType, space numberSpace, k fixedKeys, buf []byte) int { +func (c *Conn) handleLongHeader(now time.Time, dgram *datagram, ptype packetType, space numberSpace, k fixedKeys, buf []byte) int { if !k.isSet() { return skipLongHeaderPacket(buf) } @@ -125,7 +125,7 @@ func (c *Conn) handleLongHeader(now time.Time, ptype packetType, space numberSpa c.logLongPacketReceived(p, buf[:n]) } c.connIDState.handlePacket(c, p.ptype, p.srcConnID) - ackEliciting := c.handleFrames(now, ptype, space, p.payload) + ackEliciting := c.handleFrames(now, dgram, ptype, space, p.payload) c.acks[space].receive(now, space, p.num, ackEliciting) if p.ptype == packetTypeHandshake && c.side == serverSide { c.loss.validateClientAddress() @@ -138,7 +138,7 @@ func (c *Conn) handleLongHeader(now time.Time, ptype packetType, space numberSpa return n } -func (c *Conn) handle1RTT(now time.Time, buf []byte) int { +func (c *Conn) handle1RTT(now time.Time, dgram *datagram, buf []byte) int { if !c.keysAppData.canRead() { // 1-RTT packets extend to the end of the datagram, // so skip the remainder of the datagram if we can't parse this. @@ -175,7 +175,7 @@ func (c *Conn) handle1RTT(now time.Time, buf []byte) int { if c.logEnabled(QLogLevelPacket) { c.log1RTTPacketReceived(p, buf) } - ackEliciting := c.handleFrames(now, packetType1RTT, appDataSpace, p.payload) + ackEliciting := c.handleFrames(now, dgram, packetType1RTT, appDataSpace, p.payload) c.acks[appDataSpace].receive(now, appDataSpace, p.num, ackEliciting) return len(buf) } @@ -252,7 +252,7 @@ func (c *Conn) handleVersionNegotiation(now time.Time, pkt []byte) { c.abortImmediately(now, errVersionNegotiation) } -func (c *Conn) handleFrames(now time.Time, ptype packetType, space numberSpace, payload []byte) (ackEliciting bool) { +func (c *Conn) handleFrames(now time.Time, dgram *datagram, ptype packetType, space numberSpace, payload []byte) (ackEliciting bool) { if len(payload) == 0 { // "An endpoint MUST treat receipt of a packet containing no frames // as a connection error of type PROTOCOL_VIOLATION." @@ -373,6 +373,16 @@ func (c *Conn) handleFrames(now time.Time, ptype packetType, space numberSpace, return } n = c.handleRetireConnectionIDFrame(now, space, payload) + case frameTypePathChallenge: + if !frameOK(c, ptype, __01) { + return + } + n = c.handlePathChallengeFrame(now, dgram, space, payload) + case frameTypePathResponse: + if !frameOK(c, ptype, ___1) { + return + } + n = c.handlePathResponseFrame(now, space, payload) case frameTypeConnectionCloseTransport: // Transport CONNECTION_CLOSE is OK in all spaces. n = c.handleConnectionCloseTransportFrame(now, payload) @@ -546,6 +556,24 @@ func (c *Conn) handleRetireConnectionIDFrame(now time.Time, space numberSpace, p return n } +func (c *Conn) handlePathChallengeFrame(now time.Time, dgram *datagram, space numberSpace, payload []byte) int { + data, n := consumePathChallengeFrame(payload) + if n < 0 { + return -1 + } + c.handlePathChallenge(now, dgram, data) + return n +} + +func (c *Conn) handlePathResponseFrame(now time.Time, space numberSpace, payload []byte) int { + data, n := consumePathResponseFrame(payload) + if n < 0 { + return -1 + } + c.handlePathResponse(now, data) + return n +} + func (c *Conn) handleConnectionCloseTransportFrame(now time.Time, payload []byte) int { code, _, reason, n := consumeConnectionCloseTransportFrame(payload) if n < 0 { diff --git a/internal/quic/conn_send.go b/internal/quic/conn_send.go index 12bcfe308..a87cac232 100644 --- a/internal/quic/conn_send.go +++ b/internal/quic/conn_send.go @@ -271,6 +271,13 @@ func (c *Conn) appendFrames(now time.Time, space numberSpace, pnum packetNumber, return } + // PATH_RESPONSE + if pad, ok := c.appendPathFrames(); !ok { + return + } else if pad { + defer c.w.appendPaddingTo(smallestMaxDatagramSize) + } + // All stream-related frames. This should come last in the packet, // so large amounts of STREAM data don't crowd out other frames // we may need to send. diff --git a/internal/quic/conn_test.go b/internal/quic/conn_test.go index a8f3fc7fd..16ee3cf2f 100644 --- a/internal/quic/conn_test.go +++ b/internal/quic/conn_test.go @@ -168,6 +168,7 @@ type testConn struct { sentDatagrams [][]byte sentPackets []*testPacket sentFrames []debugFrame + lastDatagram *testDatagram lastPacket *testPacket recvDatagram chan *datagram @@ -576,6 +577,7 @@ func (tc *testConn) readDatagram() *testDatagram { } p.frames = frames } + tc.lastDatagram = d return d } diff --git a/internal/quic/frame_debug.go b/internal/quic/frame_debug.go index 0902c385f..17234dd7c 100644 --- a/internal/quic/frame_debug.go +++ b/internal/quic/frame_debug.go @@ -77,6 +77,7 @@ func parseDebugFrame(b []byte) (f debugFrame, n int) { // debugFramePadding is a sequence of PADDING frames. type debugFramePadding struct { size int + to int // alternate for writing packets: pad to } func parseDebugFramePadding(b []byte) (f debugFramePadding, n int) { @@ -95,6 +96,10 @@ func (f debugFramePadding) write(w *packetWriter) bool { if w.avail() == 0 { return false } + if f.to > 0 { + w.appendPaddingTo(f.to) + return true + } for i := 0; i < f.size && w.avail() > 0; i++ { w.b = append(w.b, frameTypePadding) } @@ -584,7 +589,7 @@ func (f debugFrameRetireConnectionID) LogValue() slog.Value { // debugFramePathChallenge is a PATH_CHALLENGE frame. type debugFramePathChallenge struct { - data uint64 + data pathChallengeData } func parseDebugFramePathChallenge(b []byte) (f debugFramePathChallenge, n int) { @@ -593,7 +598,7 @@ func parseDebugFramePathChallenge(b []byte) (f debugFramePathChallenge, n int) { } func (f debugFramePathChallenge) String() string { - return fmt.Sprintf("PATH_CHALLENGE Data=%016x", f.data) + return fmt.Sprintf("PATH_CHALLENGE Data=%x", f.data) } func (f debugFramePathChallenge) write(w *packetWriter) bool { @@ -603,13 +608,13 @@ func (f debugFramePathChallenge) write(w *packetWriter) bool { func (f debugFramePathChallenge) LogValue() slog.Value { return slog.GroupValue( slog.String("frame_type", "path_challenge"), - slog.String("data", fmt.Sprintf("%016x", f.data)), + slog.String("data", fmt.Sprintf("%x", f.data)), ) } // debugFramePathResponse is a PATH_RESPONSE frame. type debugFramePathResponse struct { - data uint64 + data pathChallengeData } func parseDebugFramePathResponse(b []byte) (f debugFramePathResponse, n int) { @@ -618,7 +623,7 @@ func parseDebugFramePathResponse(b []byte) (f debugFramePathResponse, n int) { } func (f debugFramePathResponse) String() string { - return fmt.Sprintf("PATH_RESPONSE Data=%016x", f.data) + return fmt.Sprintf("PATH_RESPONSE Data=%x", f.data) } func (f debugFramePathResponse) write(w *packetWriter) bool { @@ -628,7 +633,7 @@ func (f debugFramePathResponse) write(w *packetWriter) bool { func (f debugFramePathResponse) LogValue() slog.Value { return slog.GroupValue( slog.String("frame_type", "path_response"), - slog.String("data", fmt.Sprintf("%016x", f.data)), + slog.String("data", fmt.Sprintf("%x", f.data)), ) } diff --git a/internal/quic/packet_codec_test.go b/internal/quic/packet_codec_test.go index 475e18c1d..98b3bbb05 100644 --- a/internal/quic/packet_codec_test.go +++ b/internal/quic/packet_codec_test.go @@ -517,7 +517,7 @@ func TestFrameEncodeDecode(t *testing.T) { s: "PATH_CHALLENGE Data=0123456789abcdef", j: `{"frame_type":"path_challenge","data":"0123456789abcdef"}`, f: debugFramePathChallenge{ - data: 0x0123456789abcdef, + data: pathChallengeData{0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef}, }, b: []byte{ 0x1a, // Type (i) = 0x1a, @@ -527,7 +527,7 @@ func TestFrameEncodeDecode(t *testing.T) { s: "PATH_RESPONSE Data=0123456789abcdef", j: `{"frame_type":"path_response","data":"0123456789abcdef"}`, f: debugFramePathResponse{ - data: 0x0123456789abcdef, + data: pathChallengeData{0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef}, }, b: []byte{ 0x1b, // Type (i) = 0x1b, diff --git a/internal/quic/packet_parser.go b/internal/quic/packet_parser.go index 02ef9fb14..feef9eac7 100644 --- a/internal/quic/packet_parser.go +++ b/internal/quic/packet_parser.go @@ -463,18 +463,17 @@ func consumeRetireConnectionIDFrame(b []byte) (seq int64, n int) { return seq, n } -func consumePathChallengeFrame(b []byte) (data uint64, n int) { +func consumePathChallengeFrame(b []byte) (data pathChallengeData, n int) { n = 1 - var nn int - data, nn = consumeUint64(b[n:]) - if nn < 0 { - return 0, -1 + nn := copy(data[:], b[n:]) + if nn != len(data) { + return data, -1 } n += nn return data, n } -func consumePathResponseFrame(b []byte) (data uint64, n int) { +func consumePathResponseFrame(b []byte) (data pathChallengeData, n int) { return consumePathChallengeFrame(b) // identical frame format } diff --git a/internal/quic/packet_writer.go b/internal/quic/packet_writer.go index 9ed393502..e4d71e622 100644 --- a/internal/quic/packet_writer.go +++ b/internal/quic/packet_writer.go @@ -243,10 +243,7 @@ func (w *packetWriter) appendPingFrame() (added bool) { return false } w.b = append(w.b, frameTypePing) - // Mark this packet as ack-eliciting and in-flight, - // but there's no need to record the presence of a PING frame in it. - w.sent.ackEliciting = true - w.sent.inFlight = true + w.sent.markAckEliciting() // no need to record the frame itself return true } @@ -495,23 +492,23 @@ func (w *packetWriter) appendRetireConnectionIDFrame(seq int64) (added bool) { return true } -func (w *packetWriter) appendPathChallengeFrame(data uint64) (added bool) { +func (w *packetWriter) appendPathChallengeFrame(data pathChallengeData) (added bool) { if w.avail() < 1+8 { return false } w.b = append(w.b, frameTypePathChallenge) - w.b = binary.BigEndian.AppendUint64(w.b, data) - w.sent.appendAckElicitingFrame(frameTypePathChallenge) + w.b = append(w.b, data[:]...) + w.sent.markAckEliciting() // no need to record the frame itself return true } -func (w *packetWriter) appendPathResponseFrame(data uint64) (added bool) { +func (w *packetWriter) appendPathResponseFrame(data pathChallengeData) (added bool) { if w.avail() < 1+8 { return false } w.b = append(w.b, frameTypePathResponse) - w.b = binary.BigEndian.AppendUint64(w.b, data) - w.sent.appendAckElicitingFrame(frameTypePathResponse) + w.b = append(w.b, data[:]...) + w.sent.markAckEliciting() // no need to record the frame itself return true } diff --git a/internal/quic/path.go b/internal/quic/path.go new file mode 100644 index 000000000..8c237dd45 --- /dev/null +++ b/internal/quic/path.go @@ -0,0 +1,89 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 + +package quic + +import "time" + +type pathState struct { + // Response to a peer's PATH_CHALLENGE. + // This is not a sentVal, because we don't resend lost PATH_RESPONSE frames. + // We only track the most recent PATH_CHALLENGE. + // If the peer sends a second PATH_CHALLENGE before we respond to the first, + // we'll drop the first response. + sendPathResponse pathResponseType + data pathChallengeData +} + +// pathChallengeData is data carried in a PATH_CHALLENGE or PATH_RESPONSE frame. +type pathChallengeData [64 / 8]byte + +type pathResponseType uint8 + +const ( + pathResponseNotNeeded = pathResponseType(iota) + pathResponseSmall // send PATH_RESPONSE, do not expand datagram + pathResponseExpanded // send PATH_RESPONSE, expand datagram to 1200 bytes +) + +func (c *Conn) handlePathChallenge(_ time.Time, dgram *datagram, data pathChallengeData) { + // A PATH_RESPONSE is sent in a datagram expanded to 1200 bytes, + // except when this would exceed the anti-amplification limit. + // + // Rather than maintaining anti-amplification state for each path + // we may be sending a PATH_RESPONSE on, follow the following heuristic: + // + // If we receive a PATH_CHALLENGE in an expanded datagram, + // respond with an expanded datagram. + // + // If we receive a PATH_CHALLENGE in a non-expanded datagram, + // then the peer is presumably blocked by its own anti-amplification limit. + // Respond with a non-expanded datagram. Receiving this PATH_RESPONSE + // will validate the path to the peer, remove its anti-amplification limit, + // and permit it to send a followup PATH_CHALLENGE in an expanded datagram. + // https://www.rfc-editor.org/rfc/rfc9000.html#section-8.2.1 + if len(dgram.b) >= smallestMaxDatagramSize { + c.path.sendPathResponse = pathResponseExpanded + } else { + c.path.sendPathResponse = pathResponseSmall + } + c.path.data = data +} + +func (c *Conn) handlePathResponse(now time.Time, _ pathChallengeData) { + // "If the content of a PATH_RESPONSE frame does not match the content of + // a PATH_CHALLENGE frame previously sent by the endpoint, + // the endpoint MAY generate a connection error of type PROTOCOL_VIOLATION." + // https://www.rfc-editor.org/rfc/rfc9000.html#section-19.18-4 + // + // We never send PATH_CHALLENGE frames. + c.abort(now, localTransportError{ + code: errProtocolViolation, + reason: "PATH_RESPONSE received when no PATH_CHALLENGE sent", + }) +} + +// appendPathFrames appends path validation related frames to the current packet. +// If the return value pad is true, then the packet should be padded to 1200 bytes. +func (c *Conn) appendPathFrames() (pad, ok bool) { + if c.path.sendPathResponse == pathResponseNotNeeded { + return pad, true + } + // We're required to send the PATH_RESPONSE on the path where the + // PATH_CHALLENGE was received (RFC 9000, Section 8.2.2). + // + // At the moment, we don't support path migration and reject packets if + // the peer changes its source address, so just sending the PATH_RESPONSE + // in a regular datagram is fine. + if !c.w.appendPathResponseFrame(c.path.data) { + return pad, false + } + if c.path.sendPathResponse == pathResponseExpanded { + pad = true + } + c.path.sendPathResponse = pathResponseNotNeeded + return pad, true +} diff --git a/internal/quic/path_test.go b/internal/quic/path_test.go new file mode 100644 index 000000000..a309ed14b --- /dev/null +++ b/internal/quic/path_test.go @@ -0,0 +1,66 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 + +package quic + +import ( + "testing" +) + +func TestPathChallengeReceived(t *testing.T) { + for _, test := range []struct { + name string + padTo int + wantPadding int + }{{ + name: "unexpanded", + padTo: 0, + wantPadding: 0, + }, { + name: "expanded", + padTo: 1200, + wantPadding: 1200, + }} { + // "The recipient of [a PATH_CHALLENGE] frame MUST generate + // a PATH_RESPONSE frame [...] containing the same Data value." + // https://www.rfc-editor.org/rfc/rfc9000.html#section-19.17-7 + tc := newTestConn(t, clientSide) + tc.handshake() + tc.ignoreFrame(frameTypeAck) + data := pathChallengeData{0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef} + tc.writeFrames(packetType1RTT, debugFramePathChallenge{ + data: data, + }, debugFramePadding{ + to: test.padTo, + }) + tc.wantFrame("response to PATH_CHALLENGE", + packetType1RTT, debugFramePathResponse{ + data: data, + }) + if got, want := tc.lastDatagram.paddedSize, test.wantPadding; got != want { + t.Errorf("PATH_RESPONSE expanded to %v bytes, want %v", got, want) + } + tc.wantIdle("connection is idle") + } +} + +func TestPathResponseMismatchReceived(t *testing.T) { + // "If the content of a PATH_RESPONSE frame does not match the content of + // a PATH_CHALLENGE frame previously sent by the endpoint, + // the endpoint MAY generate a connection error of type PROTOCOL_VIOLATION." + // https://www.rfc-editor.org/rfc/rfc9000.html#section-19.18-4 + tc := newTestConn(t, clientSide) + tc.handshake() + tc.ignoreFrame(frameTypeAck) + tc.writeFrames(packetType1RTT, debugFramePathResponse{ + data: pathChallengeData{}, + }) + tc.wantFrame("invalid PATH_RESPONSE causes the connection to close", + packetType1RTT, debugFrameConnectionCloseTransport{ + code: errProtocolViolation, + }, + ) +} diff --git a/internal/quic/sent_packet.go b/internal/quic/sent_packet.go index 194cdc9fa..226152327 100644 --- a/internal/quic/sent_packet.go +++ b/internal/quic/sent_packet.go @@ -59,6 +59,12 @@ func (sent *sentPacket) reset() { } } +// markAckEliciting marks the packet as containing an ack-eliciting frame. +func (sent *sentPacket) markAckEliciting() { + sent.ackEliciting = true + sent.inFlight = true +} + // The append* methods record information about frames in the packet. func (sent *sentPacket) appendNonAckElicitingFrame(frameType byte) { From 22cbde9a565f4e40b5060a41d5e5171adcff673e Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 20 Feb 2024 14:58:00 -0800 Subject: [PATCH 14/22] quic: set ServerName in client connection TLSConfig Client connections must set tls.Config.ServerName to authenticate the identity of the server. (RFC 9001, Section 4.4.) Previously, we specified a single tls.Config per Endpoint. Change the Config passed to Listen to only apply to client connections accepted by the endpoint. Add a Config parameter to Listener.Dial to allow specifying a separate config per outbound connection, allowing the user to set the ServerName field. When the user does not set ServerName, set it ourselves. For golang/go#58547 Change-Id: Ie2500ae7c7a85400e6cc1c10cefa2bd4c746e313 Reviewed-on: https://go-review.googlesource.com/c/net/+/565796 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/cmd/interop/main.go | 6 ++-- internal/quic/config.go | 7 ++++ internal/quic/conn.go | 4 +-- internal/quic/conn_test.go | 2 ++ internal/quic/endpoint.go | 59 ++++++++++++++++++------------- internal/quic/endpoint_test.go | 31 ++++++++++------ internal/quic/tls.go | 14 ++++++-- 7 files changed, 81 insertions(+), 42 deletions(-) diff --git a/internal/quic/cmd/interop/main.go b/internal/quic/cmd/interop/main.go index 20f737b52..0899e0f1e 100644 --- a/internal/quic/cmd/interop/main.go +++ b/internal/quic/cmd/interop/main.go @@ -148,7 +148,7 @@ func basicTest(ctx context.Context, config *quic.Config, urls []string) { g.Add(1) go func() { defer g.Done() - fetchFrom(ctx, l, addr, u) + fetchFrom(ctx, config, l, addr, u) }() } @@ -221,8 +221,8 @@ func parseURL(s string) (u *url.URL, authority string, err error) { return u, authority, nil } -func fetchFrom(ctx context.Context, l *quic.Endpoint, addr string, urls []*url.URL) { - conn, err := l.Dial(ctx, "udp", addr) +func fetchFrom(ctx context.Context, config *quic.Config, l *quic.Endpoint, addr string, urls []*url.URL) { + conn, err := l.Dial(ctx, "udp", addr, config) if err != nil { log.Printf("%v: %v", addr, err) return diff --git a/internal/quic/config.go b/internal/quic/config.go index b045b7b92..5d420312b 100644 --- a/internal/quic/config.go +++ b/internal/quic/config.go @@ -107,6 +107,13 @@ type Config struct { QLogLogger *slog.Logger } +// Clone returns a shallow clone of c, or nil if c is nil. +// It is safe to clone a [Config] that is being used concurrently by a QUIC endpoint. +func (c *Config) Clone() *Config { + n := *c + return &n +} + func configDefault[T ~int64](v, def, limit T) T { switch { case v == 0: diff --git a/internal/quic/conn.go b/internal/quic/conn.go index d462e9617..38e8fe8f4 100644 --- a/internal/quic/conn.go +++ b/internal/quic/conn.go @@ -94,7 +94,7 @@ type newServerConnIDs struct { retrySrcConnID []byte // source from server's Retry } -func newConn(now time.Time, side connSide, cids newServerConnIDs, peerAddr netip.AddrPort, config *Config, e *Endpoint) (conn *Conn, _ error) { +func newConn(now time.Time, side connSide, cids newServerConnIDs, peerHostname string, peerAddr netip.AddrPort, config *Config, e *Endpoint) (conn *Conn, _ error) { c := &Conn{ side: side, endpoint: e, @@ -146,7 +146,7 @@ func newConn(now time.Time, side connSide, cids newServerConnIDs, peerAddr netip c.lifetimeInit() c.restartIdleTimer(now) - if err := c.startTLS(now, initialConnID, transportParameters{ + if err := c.startTLS(now, initialConnID, peerHostname, transportParameters{ initialSrcConnID: c.connIDState.srcConnID(), originalDstConnID: cids.originalDstConnID, retrySrcConnID: cids.retrySrcConnID, diff --git a/internal/quic/conn_test.go b/internal/quic/conn_test.go index 16ee3cf2f..a765ad60c 100644 --- a/internal/quic/conn_test.go +++ b/internal/quic/conn_test.go @@ -242,8 +242,10 @@ func newTestConn(t *testing.T, side connSide, opts ...any) *testConn { endpoint.configTestConn = configTestConn conn, err := endpoint.e.newConn( endpoint.now, + config, side, cids, + "", netip.MustParseAddrPort("127.0.0.1:443")) if err != nil { t.Fatal(err) diff --git a/internal/quic/endpoint.go b/internal/quic/endpoint.go index 6631708b8..a55336b24 100644 --- a/internal/quic/endpoint.go +++ b/internal/quic/endpoint.go @@ -22,11 +22,11 @@ import ( // // Multiple goroutines may invoke methods on an Endpoint simultaneously. type Endpoint struct { - config *Config - packetConn packetConn - testHooks endpointTestHooks - resetGen statelessResetTokenGenerator - retry retryState + listenConfig *Config + packetConn packetConn + testHooks endpointTestHooks + resetGen statelessResetTokenGenerator + retry retryState acceptQueue queue[*Conn] // new inbound connections connsMap connsMap // only accessed by the listen loop @@ -51,9 +51,11 @@ type packetConn interface { } // Listen listens on a local network address. -// The configuration config must be non-nil. -func Listen(network, address string, config *Config) (*Endpoint, error) { - if config.TLSConfig == nil { +// +// The config is used to for connections accepted by the endpoint. +// If the config is nil, the endpoint will not accept connections. +func Listen(network, address string, listenConfig *Config) (*Endpoint, error) { + if listenConfig != nil && listenConfig.TLSConfig == nil { return nil, errors.New("TLSConfig is not set") } a, err := net.ResolveUDPAddr(network, address) @@ -68,21 +70,25 @@ func Listen(network, address string, config *Config) (*Endpoint, error) { if err != nil { return nil, err } - return newEndpoint(pc, config, nil) + return newEndpoint(pc, listenConfig, nil) } func newEndpoint(pc packetConn, config *Config, hooks endpointTestHooks) (*Endpoint, error) { e := &Endpoint{ - config: config, - packetConn: pc, - testHooks: hooks, - conns: make(map[*Conn]struct{}), - acceptQueue: newQueue[*Conn](), - closec: make(chan struct{}), - } - e.resetGen.init(config.StatelessResetKey) + listenConfig: config, + packetConn: pc, + testHooks: hooks, + conns: make(map[*Conn]struct{}), + acceptQueue: newQueue[*Conn](), + closec: make(chan struct{}), + } + var statelessResetKey [32]byte + if config != nil { + statelessResetKey = config.StatelessResetKey + } + e.resetGen.init(statelessResetKey) e.connsMap.init() - if config.RequireAddressValidation { + if config != nil && config.RequireAddressValidation { if err := e.retry.init(); err != nil { return nil, err } @@ -141,14 +147,15 @@ func (e *Endpoint) Accept(ctx context.Context) (*Conn, error) { } // Dial creates and returns a connection to a network address. -func (e *Endpoint) Dial(ctx context.Context, network, address string) (*Conn, error) { +// The config cannot be nil. +func (e *Endpoint) Dial(ctx context.Context, network, address string, config *Config) (*Conn, error) { u, err := net.ResolveUDPAddr(network, address) if err != nil { return nil, err } addr := u.AddrPort() addr = netip.AddrPortFrom(addr.Addr().Unmap(), addr.Port()) - c, err := e.newConn(time.Now(), clientSide, newServerConnIDs{}, addr) + c, err := e.newConn(time.Now(), config, clientSide, newServerConnIDs{}, address, addr) if err != nil { return nil, err } @@ -159,13 +166,13 @@ func (e *Endpoint) Dial(ctx context.Context, network, address string) (*Conn, er return c, nil } -func (e *Endpoint) newConn(now time.Time, side connSide, cids newServerConnIDs, peerAddr netip.AddrPort) (*Conn, error) { +func (e *Endpoint) newConn(now time.Time, config *Config, side connSide, cids newServerConnIDs, peerHostname string, peerAddr netip.AddrPort) (*Conn, error) { e.connsMu.Lock() defer e.connsMu.Unlock() if e.closing { return nil, errors.New("endpoint closed") } - c, err := newConn(now, side, cids, peerAddr, e.config, e) + c, err := newConn(now, side, cids, peerHostname, peerAddr, config, e) if err != nil { return nil, err } @@ -288,11 +295,15 @@ func (e *Endpoint) handleUnknownDestinationDatagram(m *datagram) { // https://www.rfc-editor.org/rfc/rfc9000#section-10.3-16 return } + if e.listenConfig == nil { + // We are not configured to accept connections. + return + } cids := newServerConnIDs{ srcConnID: p.srcConnID, dstConnID: p.dstConnID, } - if e.config.RequireAddressValidation { + if e.listenConfig.RequireAddressValidation { var ok bool cids.retrySrcConnID = p.dstConnID cids.originalDstConnID, ok = e.validateInitialAddress(now, p, m.peerAddr) @@ -303,7 +314,7 @@ func (e *Endpoint) handleUnknownDestinationDatagram(m *datagram) { cids.originalDstConnID = p.dstConnID } var err error - c, err := e.newConn(now, serverSide, cids, m.peerAddr) + c, err := e.newConn(now, e.listenConfig, serverSide, cids, "", m.peerAddr) if err != nil { // The accept queue is probably full. // We could send a CONNECTION_CLOSE to the peer to reject the connection. diff --git a/internal/quic/endpoint_test.go b/internal/quic/endpoint_test.go index b9fb55fb3..b6669fc83 100644 --- a/internal/quic/endpoint_test.go +++ b/internal/quic/endpoint_test.go @@ -67,7 +67,8 @@ func newLocalConnPair(t testing.TB, conf1, conf2 *Config) (clientConn, serverCon ctx := context.Background() e1 := newLocalEndpoint(t, serverSide, conf1) e2 := newLocalEndpoint(t, clientSide, conf2) - c2, err := e2.Dial(ctx, "udp", e1.LocalAddr().String()) + conf2 = makeTestConfig(conf2, clientSide) + c2, err := e2.Dial(ctx, "udp", e1.LocalAddr().String(), conf2) if err != nil { t.Fatal(err) } @@ -80,9 +81,24 @@ func newLocalConnPair(t testing.TB, conf1, conf2 *Config) (clientConn, serverCon func newLocalEndpoint(t testing.TB, side connSide, conf *Config) *Endpoint { t.Helper() + conf = makeTestConfig(conf, side) + e, err := Listen("udp", "127.0.0.1:0", conf) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + e.Close(canceledContext()) + }) + return e +} + +func makeTestConfig(conf *Config, side connSide) *Config { + if conf == nil { + return nil + } + newConf := *conf + conf = &newConf if conf.TLSConfig == nil { - newConf := *conf - conf = &newConf conf.TLSConfig = newTestTLSConfig(side) } if conf.QLogLogger == nil { @@ -91,14 +107,7 @@ func newLocalEndpoint(t testing.TB, side connSide, conf *Config) *Endpoint { Dir: *qlogdir, })) } - e, err := Listen("udp", "127.0.0.1:0", conf) - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { - e.Close(canceledContext()) - }) - return e + return conf } type testEndpoint struct { diff --git a/internal/quic/tls.go b/internal/quic/tls.go index a37e26fb8..e2f2e5bde 100644 --- a/internal/quic/tls.go +++ b/internal/quic/tls.go @@ -11,14 +11,24 @@ import ( "crypto/tls" "errors" "fmt" + "net" "time" ) // startTLS starts the TLS handshake. -func (c *Conn) startTLS(now time.Time, initialConnID []byte, params transportParameters) error { +func (c *Conn) startTLS(now time.Time, initialConnID []byte, peerHostname string, params transportParameters) error { + tlsConfig := c.config.TLSConfig + if a, _, err := net.SplitHostPort(peerHostname); err == nil { + peerHostname = a + } + if tlsConfig.ServerName == "" && peerHostname != "" { + tlsConfig = tlsConfig.Clone() + tlsConfig.ServerName = peerHostname + } + c.keysInitial = initialKeys(initialConnID, c.side) - qconfig := &tls.QUICConfig{TLSConfig: c.config.TLSConfig} + qconfig := &tls.QUICConfig{TLSConfig: tlsConfig} if c.side == clientSide { c.tls = tls.QUICClient(qconfig) } else { From 4bdc6df28ea746166f486314f8848eb9b25b9073 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 14 Feb 2024 11:22:21 -0800 Subject: [PATCH 15/22] quic: expand package docs, and document Stream For golang/go#58547 Change-Id: Ie5dd0ed383ea7a5b3a45103cb730ff62792f62e1 Reviewed-on: https://go-review.googlesource.com/c/net/+/565797 Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: Go LUCI --- internal/quic/doc.go | 42 ++++++++++++++++++++++++++++++++++++++--- internal/quic/stream.go | 15 +++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/internal/quic/doc.go b/internal/quic/doc.go index 2fe17fe22..2fd10f087 100644 --- a/internal/quic/doc.go +++ b/internal/quic/doc.go @@ -2,8 +2,44 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Package quic is an experimental, incomplete implementation of the QUIC protocol. -// This package is a work in progress, and is not ready for use at this time. +// Package quic implements the QUIC protocol. // -// This package implements (or will implement) RFC 9000, RFC 9001, and RFC 9002. +// This package is a work in progress. +// It is not ready for production usage. +// Its API is subject to change without notice. +// +// This package is low-level. +// Most users will use it indirectly through an HTTP/3 implementation. +// +// # Usage +// +// An [Endpoint] sends and receives traffic on a network address. +// Create an Endpoint to either accept inbound QUIC connections +// or create outbound ones. +// +// A [Conn] is a QUIC connection. +// +// A [Stream] is a QUIC stream, an ordered, reliable byte stream. +// +// # Cancelation +// +// All blocking operations may be canceled using a context.Context. +// When performing an operation with a canceled context, the operation +// will succeed if doing so does not require blocking. For example, +// reading from a stream will return data when buffered data is available, +// even if the stream context is canceled. +// +// # Limitations +// +// This package is a work in progress. +// Known limitations include: +// +// - Performance is untuned. +// - 0-RTT is not supported. +// - Address migration is not supported. +// - Server preferred addresses are not supported. +// - The latency spin bit is not supported. +// - Stream send/receive windows are configurable, +// but are fixed and do not adapt to available throughput. +// - Path MTU discovery is not implemented. package quic diff --git a/internal/quic/stream.go b/internal/quic/stream.go index c5fafdf1d..cb45534f8 100644 --- a/internal/quic/stream.go +++ b/internal/quic/stream.go @@ -14,6 +14,21 @@ import ( "math" ) +// A Stream is an ordered byte stream. +// +// Streams may be bidirectional, read-only, or write-only. +// Methods inappropriate for a stream's direction +// (for example, [Write] to a read-only stream) +// return errors. +// +// It is not safe to perform concurrent reads from or writes to a stream. +// It is safe, however, to read and write at the same time. +// +// Reads and writes are buffered. +// It is generally not necessary to wrap a stream in a [bufio.ReadWriter] +// or otherwise apply additional buffering. +// +// To cancel reads or writes, use the [SetReadContext] and [SetWriteContext] methods. type Stream struct { id streamID conn *Conn From 34cc4464c5cb7947126d80f9d75b4c16d229337d Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 23 Feb 2024 08:53:01 -0800 Subject: [PATCH 16/22] quic: temporarily disable networking tests failing on various platforms For golang/go#65906 For golang/go#65907 Change-Id: I5fe83a27f47b6f2337d280465bf134dbd883809d Reviewed-on: https://go-review.googlesource.com/c/net/+/566098 Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Bryan Mills --- internal/quic/udp_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/quic/udp_test.go b/internal/quic/udp_test.go index 27eddf811..450351b6b 100644 --- a/internal/quic/udp_test.go +++ b/internal/quic/udp_test.go @@ -16,6 +16,7 @@ import ( ) func TestUDPSourceUnspecified(t *testing.T) { + t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with no source address set. runUDPTest(t, func(t *testing.T, test udpTest) { data := []byte("source unspecified") @@ -33,6 +34,7 @@ func TestUDPSourceUnspecified(t *testing.T) { } func TestUDPSourceSpecified(t *testing.T) { + t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with source address set. runUDPTest(t, func(t *testing.T, test udpTest) { data := []byte("source specified") @@ -51,6 +53,7 @@ func TestUDPSourceSpecified(t *testing.T) { } func TestUDPSourceInvalid(t *testing.T) { + t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with source address set to an address not associated with the connection. if !udpInvalidLocalAddrIsError { t.Skipf("%v: sending from invalid source succeeds", runtime.GOOS) @@ -74,6 +77,7 @@ func TestUDPSourceInvalid(t *testing.T) { } func TestUDPECN(t *testing.T) { + t.Skip("https://go.dev/issue/65907 - temporarily skipped pending fix") if !udpECNSupport { t.Skipf("%v: no ECN support", runtime.GOOS) } From 591be7f10be18b4b24250868fb61a93c2e5af3f4 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 23 Feb 2024 09:54:56 -0800 Subject: [PATCH 17/22] quic: fix UDP on big-endian Linux, tests on various architectures The following cmsgs contain a native-endian 32-bit integer: - IP_TOS, passed to sendmsg - IPV6_TCLASS, always IP_TOS received from recvmsg contains a single byte, because why not. We were inadvertently assuming little-endian integers in all cases. Add endianness conversion as appropriate. Disable tests that rely on IPv4-in-IPv6 mapped sockets on dragonfly and openbsd, which don't support this feature. (A "udp" socket cannot receive IPv6 packets on these platforms.) Disable IPv6 tests on wasm, where the simulated networking appears to generally not support IPv6. Fixes golang/go#65906 Fixes golang/go#65907 Change-Id: Ie50af12e182a1a5d685ce4fbdf008748f6aee339 Reviewed-on: https://go-review.googlesource.com/c/net/+/566296 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam Reviewed-by: Bryan Mills --- internal/quic/udp_darwin.go | 25 +++++++++++ internal/quic/udp_linux.go | 20 +++++++++ internal/quic/udp_msg.go | 89 ++++++++++++++++++------------------- internal/quic/udp_test.go | 17 +++++-- 4 files changed, 102 insertions(+), 49 deletions(-) diff --git a/internal/quic/udp_darwin.go b/internal/quic/udp_darwin.go index 3868a36a8..2eb2e9f9f 100644 --- a/internal/quic/udp_darwin.go +++ b/internal/quic/udp_darwin.go @@ -6,8 +6,33 @@ package quic +import ( + "encoding/binary" + + "golang.org/x/sys/unix" +) + // See udp.go. const ( udpECNSupport = true udpInvalidLocalAddrIsError = true ) + +// Confusingly, on Darwin the contents of the IP_TOS option differ depending on whether +// it is used as an inbound or outbound cmsg. + +func parseIPTOS(b []byte) (ecnBits, bool) { + // Single byte. The low two bits are the ECN field. + if len(b) != 1 { + return 0, false + } + return ecnBits(b[0] & ecnMask), true +} + +func appendCmsgECNv4(b []byte, ecn ecnBits) []byte { + // 32-bit integer. + // https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/bsd/netinet/in_tclass.c#L1062-L1073 + b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_TOS, 4) + binary.NativeEndian.PutUint32(data, uint32(ecn)) + return b +} diff --git a/internal/quic/udp_linux.go b/internal/quic/udp_linux.go index 2ba3e6f2f..6f191ed39 100644 --- a/internal/quic/udp_linux.go +++ b/internal/quic/udp_linux.go @@ -6,8 +6,28 @@ package quic +import ( + "golang.org/x/sys/unix" +) + // See udp.go. const ( udpECNSupport = true udpInvalidLocalAddrIsError = false ) + +// The IP_TOS socket option is a single byte containing the IP TOS field. +// The low two bits are the ECN field. + +func parseIPTOS(b []byte) (ecnBits, bool) { + if len(b) != 1 { + return 0, false + } + return ecnBits(b[0] & ecnMask), true +} + +func appendCmsgECNv4(b []byte, ecn ecnBits) []byte { + b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_TOS, 1) + data[0] = byte(ecn) + return b +} diff --git a/internal/quic/udp_msg.go b/internal/quic/udp_msg.go index bdc1b710d..0b600a2b4 100644 --- a/internal/quic/udp_msg.go +++ b/internal/quic/udp_msg.go @@ -7,6 +7,7 @@ package quic import ( + "encoding/binary" "net" "net/netip" "sync" @@ -141,15 +142,11 @@ func parseControl(d *datagram, control []byte) { case unix.IPPROTO_IP: switch hdr.Type { case unix.IP_TOS, unix.IP_RECVTOS: - // Single byte containing the IP TOS field. - // The low two bits are the ECN field. - // // (Linux sets the type to IP_TOS, Darwin to IP_RECVTOS, - // jus check for both.) - if len(data) < 1 { - break + // just check for both.) + if ecn, ok := parseIPTOS(data); ok { + d.ecn = ecn } - d.ecn = ecnBits(data[0] & ecnMask) case unix.IP_PKTINFO: if a, ok := parseInPktinfo(data); ok { d.localAddr = netip.AddrPortFrom(a, d.localAddr.Port()) @@ -158,12 +155,11 @@ func parseControl(d *datagram, control []byte) { case unix.IPPROTO_IPV6: switch hdr.Type { case unix.IPV6_TCLASS: - // Single byte containing the traffic class field. + // 32-bit integer containing the traffic class field. // The low two bits are the ECN field. - if len(data) < 1 { - break + if ecn, ok := parseIPv6TCLASS(data); ok { + d.ecn = ecn } - d.ecn = ecnBits(data[0] & ecnMask) case unix.IPV6_PKTINFO: if a, ok := parseIn6Pktinfo(data); ok { d.localAddr = netip.AddrPortFrom(a, d.localAddr.Port()) @@ -173,27 +169,33 @@ func parseControl(d *datagram, control []byte) { } } -func parseInPktinfo(b []byte) (netip.Addr, bool) { - // struct in_pktinfo { - // unsigned int ipi_ifindex; /* send/recv interface index */ - // struct in_addr ipi_spec_dst; /* Local address */ - // struct in_addr ipi_addr; /* IP Header dst address */ - // }; - if len(b) != 12 { - return netip.Addr{}, false +// IPV6_TCLASS is specified by RFC 3542 as an int. + +func parseIPv6TCLASS(b []byte) (ecnBits, bool) { + if len(b) != 4 { + return 0, false } - return netip.AddrFrom4([4]byte(b[8:][:4])), true + return ecnBits(binary.NativeEndian.Uint32(b) & ecnMask), true } -func parseIn6Pktinfo(b []byte) (netip.Addr, bool) { - // struct in6_pktinfo { - // struct in6_addr ipi6_addr; /* src/dst IPv6 address */ - // unsigned int ipi6_ifindex; /* send/recv interface index */ - // }; - if len(b) != 20 { +func appendCmsgECNv6(b []byte, ecn ecnBits) []byte { + b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_TCLASS, 4) + binary.NativeEndian.PutUint32(data, uint32(ecn)) + return b +} + +// struct in_pktinfo { +// unsigned int ipi_ifindex; /* send/recv interface index */ +// struct in_addr ipi_spec_dst; /* Local address */ +// struct in_addr ipi_addr; /* IP Header dst address */ +// }; + +// parseInPktinfo returns the destination address from an IP_PKTINFO. +func parseInPktinfo(b []byte) (dst netip.Addr, ok bool) { + if len(b) != 12 { return netip.Addr{}, false } - return netip.AddrFrom16([16]byte(b[:16])).Unmap(), true + return netip.AddrFrom4([4]byte(b[8:][:4])), true } // appendCmsgIPSourceAddrV4 appends an IP_PKTINFO setting the source address @@ -210,31 +212,28 @@ func appendCmsgIPSourceAddrV4(b []byte, src netip.Addr) []byte { return b } -// appendCmsgIPSourceAddrV6 appends an IP_PKTINFO or IPV6_PKTINFO -// setting the source address for an outbound datagram. +// struct in6_pktinfo { +// struct in6_addr ipi6_addr; /* src/dst IPv6 address */ +// unsigned int ipi6_ifindex; /* send/recv interface index */ +// }; + +// parseIn6Pktinfo returns the destination address from an IPV6_PKTINFO. +func parseIn6Pktinfo(b []byte) (netip.Addr, bool) { + if len(b) != 20 { + return netip.Addr{}, false + } + return netip.AddrFrom16([16]byte(b[:16])).Unmap(), true +} + +// appendCmsgIPSourceAddrV6 appends an IPV6_PKTINFO setting the source address +// for an outbound datagram. func appendCmsgIPSourceAddrV6(b []byte, src netip.Addr) []byte { - // struct in6_pktinfo { - // struct in6_addr ipi6_addr; /* src/dst IPv6 address */ - // unsigned int ipi6_ifindex; /* send/recv interface index */ - // }; b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_PKTINFO, 20) ip := src.As16() copy(data[0:], ip[:]) return b } -func appendCmsgECNv4(b []byte, ecn ecnBits) []byte { - b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_TOS, 4) - data[0] = byte(ecn) - return b -} - -func appendCmsgECNv6(b []byte, ecn ecnBits) []byte { - b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_TCLASS, 4) - data[0] = byte(ecn) - return b -} - // appendCmsg appends a cmsg with the given level, type, and size to b. // It returns the new buffer, and the data section of the cmsg. func appendCmsg(b []byte, level, typ int32, size int) (_, data []byte) { diff --git a/internal/quic/udp_test.go b/internal/quic/udp_test.go index 450351b6b..d3732c140 100644 --- a/internal/quic/udp_test.go +++ b/internal/quic/udp_test.go @@ -16,9 +16,9 @@ import ( ) func TestUDPSourceUnspecified(t *testing.T) { - t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with no source address set. runUDPTest(t, func(t *testing.T, test udpTest) { + t.Logf("%v", test.dstAddr) data := []byte("source unspecified") if err := test.src.Write(datagram{ b: data, @@ -34,7 +34,6 @@ func TestUDPSourceUnspecified(t *testing.T) { } func TestUDPSourceSpecified(t *testing.T) { - t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with source address set. runUDPTest(t, func(t *testing.T, test udpTest) { data := []byte("source specified") @@ -53,7 +52,6 @@ func TestUDPSourceSpecified(t *testing.T) { } func TestUDPSourceInvalid(t *testing.T) { - t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with source address set to an address not associated with the connection. if !udpInvalidLocalAddrIsError { t.Skipf("%v: sending from invalid source succeeds", runtime.GOOS) @@ -77,7 +75,6 @@ func TestUDPSourceInvalid(t *testing.T) { } func TestUDPECN(t *testing.T) { - t.Skip("https://go.dev/issue/65907 - temporarily skipped pending fix") if !udpECNSupport { t.Skipf("%v: no ECN support", runtime.GOOS) } @@ -125,6 +122,18 @@ func runUDPTest(t *testing.T, f func(t *testing.T, u udpTest)) { spec = "unspec" } t.Run(fmt.Sprintf("%v/%v/%v", test.srcNet, test.dstNet, spec), func(t *testing.T) { + // See: https://go.googlesource.com/go/+/refs/tags/go1.22.0/src/net/ipsock.go#47 + // On these platforms, conns with network="udp" cannot accept IPv6. + switch runtime.GOOS { + case "dragonfly", "openbsd": + if test.srcNet == "udp6" && test.dstNet == "udp" { + t.Skipf("%v: no support for mapping IPv4 address to IPv6", runtime.GOOS) + } + } + if runtime.GOARCH == "wasm" && test.srcNet == "udp6" { + t.Skipf("%v: IPv6 tests fail when using wasm fake net", runtime.GOARCH) + } + srcAddr := netip.AddrPortFrom(netip.MustParseAddr(test.srcAddr), 0) srcConn, err := net.ListenUDP(test.srcNet, net.UDPAddrFromAddrPort(srcAddr)) if err != nil { From fa1142799318d3fa3632ecfd9f318ffa040e7c4c Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 22 Feb 2024 17:31:38 -0800 Subject: [PATCH 18/22] quic: move package out of internal For golang/go#58547 Change-Id: I119d820824f82bfdd236c6826f960d0c934745ca Reviewed-on: https://go-review.googlesource.com/c/net/+/566295 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- internal/quic/cmd/interop/main.go | 4 ++-- {internal/quic => quic}/ack_delay.go | 0 {internal/quic => quic}/ack_delay_test.go | 0 {internal/quic => quic}/acks.go | 0 {internal/quic => quic}/acks_test.go | 0 {internal/quic => quic}/atomic_bits.go | 0 {internal/quic => quic}/bench_test.go | 0 {internal/quic => quic}/config.go | 0 {internal/quic => quic}/config_test.go | 0 {internal/quic => quic}/congestion_reno.go | 0 {internal/quic => quic}/congestion_reno_test.go | 0 {internal/quic => quic}/conn.go | 0 {internal/quic => quic}/conn_async_test.go | 0 {internal/quic => quic}/conn_close.go | 0 {internal/quic => quic}/conn_close_test.go | 0 {internal/quic => quic}/conn_flow.go | 0 {internal/quic => quic}/conn_flow_test.go | 0 {internal/quic => quic}/conn_id.go | 0 {internal/quic => quic}/conn_id_test.go | 0 {internal/quic => quic}/conn_loss.go | 0 {internal/quic => quic}/conn_loss_test.go | 0 {internal/quic => quic}/conn_recv.go | 0 {internal/quic => quic}/conn_send.go | 0 {internal/quic => quic}/conn_send_test.go | 0 {internal/quic => quic}/conn_streams.go | 0 {internal/quic => quic}/conn_streams_test.go | 0 {internal/quic => quic}/conn_test.go | 2 +- {internal/quic => quic}/crypto_stream.go | 0 {internal/quic => quic}/crypto_stream_test.go | 0 {internal/quic => quic}/dgram.go | 0 {internal/quic => quic}/doc.go | 0 {internal/quic => quic}/endpoint.go | 0 {internal/quic => quic}/endpoint_test.go | 2 +- {internal/quic => quic}/errors.go | 0 {internal/quic => quic}/files_test.go | 0 {internal/quic => quic}/frame_debug.go | 0 {internal/quic => quic}/gate.go | 0 {internal/quic => quic}/gate_test.go | 0 {internal/quic => quic}/gotraceback_test.go | 0 {internal/quic => quic}/idle.go | 0 {internal/quic => quic}/idle_test.go | 0 {internal/quic => quic}/key_update_test.go | 0 {internal/quic => quic}/log.go | 0 {internal/quic => quic}/loss.go | 0 {internal/quic => quic}/loss_test.go | 0 {internal/quic => quic}/main_test.go | 0 {internal/quic => quic}/math.go | 0 {internal/quic => quic}/pacer.go | 0 {internal/quic => quic}/pacer_test.go | 0 {internal/quic => quic}/packet.go | 0 {internal/quic => quic}/packet_codec_test.go | 2 +- {internal/quic => quic}/packet_number.go | 0 {internal/quic => quic}/packet_number_test.go | 0 {internal/quic => quic}/packet_parser.go | 0 {internal/quic => quic}/packet_protection.go | 0 {internal/quic => quic}/packet_protection_test.go | 0 {internal/quic => quic}/packet_test.go | 0 {internal/quic => quic}/packet_writer.go | 0 {internal/quic => quic}/path.go | 0 {internal/quic => quic}/path_test.go | 0 {internal/quic => quic}/ping.go | 0 {internal/quic => quic}/ping_test.go | 0 {internal/quic => quic}/pipe.go | 0 {internal/quic => quic}/pipe_test.go | 0 {internal/quic => quic}/qlog.go | 0 {internal/quic => quic}/qlog/handler.go | 0 {internal/quic => quic}/qlog/json_writer.go | 0 {internal/quic => quic}/qlog/json_writer_test.go | 0 {internal/quic => quic}/qlog/qlog.go | 0 {internal/quic => quic}/qlog/qlog_test.go | 0 {internal/quic => quic}/qlog_test.go | 2 +- {internal/quic => quic}/queue.go | 0 {internal/quic => quic}/queue_test.go | 0 {internal/quic => quic}/quic.go | 0 {internal/quic => quic}/quic_test.go | 0 {internal/quic => quic}/rangeset.go | 0 {internal/quic => quic}/rangeset_test.go | 0 {internal/quic => quic}/retry.go | 0 {internal/quic => quic}/retry_test.go | 0 {internal/quic => quic}/rtt.go | 0 {internal/quic => quic}/rtt_test.go | 0 {internal/quic => quic}/sent_packet.go | 0 {internal/quic => quic}/sent_packet_list.go | 0 {internal/quic => quic}/sent_packet_list_test.go | 0 {internal/quic => quic}/sent_packet_test.go | 0 {internal/quic => quic}/sent_val.go | 0 {internal/quic => quic}/sent_val_test.go | 0 {internal/quic => quic}/stateless_reset.go | 0 {internal/quic => quic}/stateless_reset_test.go | 0 {internal/quic => quic}/stream.go | 0 {internal/quic => quic}/stream_limits.go | 0 {internal/quic => quic}/stream_limits_test.go | 0 {internal/quic => quic}/stream_test.go | 0 {internal/quic => quic}/tls.go | 0 {internal/quic => quic}/tls_test.go | 0 {internal/quic => quic}/tlsconfig_test.go | 0 {internal/quic => quic}/transport_params.go | 0 {internal/quic => quic}/transport_params_test.go | 0 {internal/quic => quic}/udp.go | 0 {internal/quic => quic}/udp_darwin.go | 0 {internal/quic => quic}/udp_linux.go | 0 {internal/quic => quic}/udp_msg.go | 0 {internal/quic => quic}/udp_other.go | 0 {internal/quic => quic}/udp_test.go | 0 {internal/quic => quic}/version_test.go | 0 {internal/quic => quic}/wire.go | 0 {internal/quic => quic}/wire_test.go | 0 107 files changed, 6 insertions(+), 6 deletions(-) rename {internal/quic => quic}/ack_delay.go (100%) rename {internal/quic => quic}/ack_delay_test.go (100%) rename {internal/quic => quic}/acks.go (100%) rename {internal/quic => quic}/acks_test.go (100%) rename {internal/quic => quic}/atomic_bits.go (100%) rename {internal/quic => quic}/bench_test.go (100%) rename {internal/quic => quic}/config.go (100%) rename {internal/quic => quic}/config_test.go (100%) rename {internal/quic => quic}/congestion_reno.go (100%) rename {internal/quic => quic}/congestion_reno_test.go (100%) rename {internal/quic => quic}/conn.go (100%) rename {internal/quic => quic}/conn_async_test.go (100%) rename {internal/quic => quic}/conn_close.go (100%) rename {internal/quic => quic}/conn_close_test.go (100%) rename {internal/quic => quic}/conn_flow.go (100%) rename {internal/quic => quic}/conn_flow_test.go (100%) rename {internal/quic => quic}/conn_id.go (100%) rename {internal/quic => quic}/conn_id_test.go (100%) rename {internal/quic => quic}/conn_loss.go (100%) rename {internal/quic => quic}/conn_loss_test.go (100%) rename {internal/quic => quic}/conn_recv.go (100%) rename {internal/quic => quic}/conn_send.go (100%) rename {internal/quic => quic}/conn_send_test.go (100%) rename {internal/quic => quic}/conn_streams.go (100%) rename {internal/quic => quic}/conn_streams_test.go (100%) rename {internal/quic => quic}/conn_test.go (99%) rename {internal/quic => quic}/crypto_stream.go (100%) rename {internal/quic => quic}/crypto_stream_test.go (100%) rename {internal/quic => quic}/dgram.go (100%) rename {internal/quic => quic}/doc.go (100%) rename {internal/quic => quic}/endpoint.go (100%) rename {internal/quic => quic}/endpoint_test.go (99%) rename {internal/quic => quic}/errors.go (100%) rename {internal/quic => quic}/files_test.go (100%) rename {internal/quic => quic}/frame_debug.go (100%) rename {internal/quic => quic}/gate.go (100%) rename {internal/quic => quic}/gate_test.go (100%) rename {internal/quic => quic}/gotraceback_test.go (100%) rename {internal/quic => quic}/idle.go (100%) rename {internal/quic => quic}/idle_test.go (100%) rename {internal/quic => quic}/key_update_test.go (100%) rename {internal/quic => quic}/log.go (100%) rename {internal/quic => quic}/loss.go (100%) rename {internal/quic => quic}/loss_test.go (100%) rename {internal/quic => quic}/main_test.go (100%) rename {internal/quic => quic}/math.go (100%) rename {internal/quic => quic}/pacer.go (100%) rename {internal/quic => quic}/pacer_test.go (100%) rename {internal/quic => quic}/packet.go (100%) rename {internal/quic => quic}/packet_codec_test.go (99%) rename {internal/quic => quic}/packet_number.go (100%) rename {internal/quic => quic}/packet_number_test.go (100%) rename {internal/quic => quic}/packet_parser.go (100%) rename {internal/quic => quic}/packet_protection.go (100%) rename {internal/quic => quic}/packet_protection_test.go (100%) rename {internal/quic => quic}/packet_test.go (100%) rename {internal/quic => quic}/packet_writer.go (100%) rename {internal/quic => quic}/path.go (100%) rename {internal/quic => quic}/path_test.go (100%) rename {internal/quic => quic}/ping.go (100%) rename {internal/quic => quic}/ping_test.go (100%) rename {internal/quic => quic}/pipe.go (100%) rename {internal/quic => quic}/pipe_test.go (100%) rename {internal/quic => quic}/qlog.go (100%) rename {internal/quic => quic}/qlog/handler.go (100%) rename {internal/quic => quic}/qlog/json_writer.go (100%) rename {internal/quic => quic}/qlog/json_writer_test.go (100%) rename {internal/quic => quic}/qlog/qlog.go (100%) rename {internal/quic => quic}/qlog/qlog_test.go (100%) rename {internal/quic => quic}/qlog_test.go (99%) rename {internal/quic => quic}/queue.go (100%) rename {internal/quic => quic}/queue_test.go (100%) rename {internal/quic => quic}/quic.go (100%) rename {internal/quic => quic}/quic_test.go (100%) rename {internal/quic => quic}/rangeset.go (100%) rename {internal/quic => quic}/rangeset_test.go (100%) rename {internal/quic => quic}/retry.go (100%) rename {internal/quic => quic}/retry_test.go (100%) rename {internal/quic => quic}/rtt.go (100%) rename {internal/quic => quic}/rtt_test.go (100%) rename {internal/quic => quic}/sent_packet.go (100%) rename {internal/quic => quic}/sent_packet_list.go (100%) rename {internal/quic => quic}/sent_packet_list_test.go (100%) rename {internal/quic => quic}/sent_packet_test.go (100%) rename {internal/quic => quic}/sent_val.go (100%) rename {internal/quic => quic}/sent_val_test.go (100%) rename {internal/quic => quic}/stateless_reset.go (100%) rename {internal/quic => quic}/stateless_reset_test.go (100%) rename {internal/quic => quic}/stream.go (100%) rename {internal/quic => quic}/stream_limits.go (100%) rename {internal/quic => quic}/stream_limits_test.go (100%) rename {internal/quic => quic}/stream_test.go (100%) rename {internal/quic => quic}/tls.go (100%) rename {internal/quic => quic}/tls_test.go (100%) rename {internal/quic => quic}/tlsconfig_test.go (100%) rename {internal/quic => quic}/transport_params.go (100%) rename {internal/quic => quic}/transport_params_test.go (100%) rename {internal/quic => quic}/udp.go (100%) rename {internal/quic => quic}/udp_darwin.go (100%) rename {internal/quic => quic}/udp_linux.go (100%) rename {internal/quic => quic}/udp_msg.go (100%) rename {internal/quic => quic}/udp_other.go (100%) rename {internal/quic => quic}/udp_test.go (100%) rename {internal/quic => quic}/version_test.go (100%) rename {internal/quic => quic}/wire.go (100%) rename {internal/quic => quic}/wire_test.go (100%) diff --git a/internal/quic/cmd/interop/main.go b/internal/quic/cmd/interop/main.go index 0899e0f1e..5b652a2b1 100644 --- a/internal/quic/cmd/interop/main.go +++ b/internal/quic/cmd/interop/main.go @@ -25,8 +25,8 @@ import ( "path/filepath" "sync" - "golang.org/x/net/internal/quic" - "golang.org/x/net/internal/quic/qlog" + "golang.org/x/net/quic" + "golang.org/x/net/quic/qlog" ) var ( diff --git a/internal/quic/ack_delay.go b/quic/ack_delay.go similarity index 100% rename from internal/quic/ack_delay.go rename to quic/ack_delay.go diff --git a/internal/quic/ack_delay_test.go b/quic/ack_delay_test.go similarity index 100% rename from internal/quic/ack_delay_test.go rename to quic/ack_delay_test.go diff --git a/internal/quic/acks.go b/quic/acks.go similarity index 100% rename from internal/quic/acks.go rename to quic/acks.go diff --git a/internal/quic/acks_test.go b/quic/acks_test.go similarity index 100% rename from internal/quic/acks_test.go rename to quic/acks_test.go diff --git a/internal/quic/atomic_bits.go b/quic/atomic_bits.go similarity index 100% rename from internal/quic/atomic_bits.go rename to quic/atomic_bits.go diff --git a/internal/quic/bench_test.go b/quic/bench_test.go similarity index 100% rename from internal/quic/bench_test.go rename to quic/bench_test.go diff --git a/internal/quic/config.go b/quic/config.go similarity index 100% rename from internal/quic/config.go rename to quic/config.go diff --git a/internal/quic/config_test.go b/quic/config_test.go similarity index 100% rename from internal/quic/config_test.go rename to quic/config_test.go diff --git a/internal/quic/congestion_reno.go b/quic/congestion_reno.go similarity index 100% rename from internal/quic/congestion_reno.go rename to quic/congestion_reno.go diff --git a/internal/quic/congestion_reno_test.go b/quic/congestion_reno_test.go similarity index 100% rename from internal/quic/congestion_reno_test.go rename to quic/congestion_reno_test.go diff --git a/internal/quic/conn.go b/quic/conn.go similarity index 100% rename from internal/quic/conn.go rename to quic/conn.go diff --git a/internal/quic/conn_async_test.go b/quic/conn_async_test.go similarity index 100% rename from internal/quic/conn_async_test.go rename to quic/conn_async_test.go diff --git a/internal/quic/conn_close.go b/quic/conn_close.go similarity index 100% rename from internal/quic/conn_close.go rename to quic/conn_close.go diff --git a/internal/quic/conn_close_test.go b/quic/conn_close_test.go similarity index 100% rename from internal/quic/conn_close_test.go rename to quic/conn_close_test.go diff --git a/internal/quic/conn_flow.go b/quic/conn_flow.go similarity index 100% rename from internal/quic/conn_flow.go rename to quic/conn_flow.go diff --git a/internal/quic/conn_flow_test.go b/quic/conn_flow_test.go similarity index 100% rename from internal/quic/conn_flow_test.go rename to quic/conn_flow_test.go diff --git a/internal/quic/conn_id.go b/quic/conn_id.go similarity index 100% rename from internal/quic/conn_id.go rename to quic/conn_id.go diff --git a/internal/quic/conn_id_test.go b/quic/conn_id_test.go similarity index 100% rename from internal/quic/conn_id_test.go rename to quic/conn_id_test.go diff --git a/internal/quic/conn_loss.go b/quic/conn_loss.go similarity index 100% rename from internal/quic/conn_loss.go rename to quic/conn_loss.go diff --git a/internal/quic/conn_loss_test.go b/quic/conn_loss_test.go similarity index 100% rename from internal/quic/conn_loss_test.go rename to quic/conn_loss_test.go diff --git a/internal/quic/conn_recv.go b/quic/conn_recv.go similarity index 100% rename from internal/quic/conn_recv.go rename to quic/conn_recv.go diff --git a/internal/quic/conn_send.go b/quic/conn_send.go similarity index 100% rename from internal/quic/conn_send.go rename to quic/conn_send.go diff --git a/internal/quic/conn_send_test.go b/quic/conn_send_test.go similarity index 100% rename from internal/quic/conn_send_test.go rename to quic/conn_send_test.go diff --git a/internal/quic/conn_streams.go b/quic/conn_streams.go similarity index 100% rename from internal/quic/conn_streams.go rename to quic/conn_streams.go diff --git a/internal/quic/conn_streams_test.go b/quic/conn_streams_test.go similarity index 100% rename from internal/quic/conn_streams_test.go rename to quic/conn_streams_test.go diff --git a/internal/quic/conn_test.go b/quic/conn_test.go similarity index 99% rename from internal/quic/conn_test.go rename to quic/conn_test.go index a765ad60c..f4f1818a6 100644 --- a/internal/quic/conn_test.go +++ b/quic/conn_test.go @@ -21,7 +21,7 @@ import ( "testing" "time" - "golang.org/x/net/internal/quic/qlog" + "golang.org/x/net/quic/qlog" ) var ( diff --git a/internal/quic/crypto_stream.go b/quic/crypto_stream.go similarity index 100% rename from internal/quic/crypto_stream.go rename to quic/crypto_stream.go diff --git a/internal/quic/crypto_stream_test.go b/quic/crypto_stream_test.go similarity index 100% rename from internal/quic/crypto_stream_test.go rename to quic/crypto_stream_test.go diff --git a/internal/quic/dgram.go b/quic/dgram.go similarity index 100% rename from internal/quic/dgram.go rename to quic/dgram.go diff --git a/internal/quic/doc.go b/quic/doc.go similarity index 100% rename from internal/quic/doc.go rename to quic/doc.go diff --git a/internal/quic/endpoint.go b/quic/endpoint.go similarity index 100% rename from internal/quic/endpoint.go rename to quic/endpoint.go diff --git a/internal/quic/endpoint_test.go b/quic/endpoint_test.go similarity index 99% rename from internal/quic/endpoint_test.go rename to quic/endpoint_test.go index b6669fc83..d5f436e6d 100644 --- a/internal/quic/endpoint_test.go +++ b/quic/endpoint_test.go @@ -16,7 +16,7 @@ import ( "testing" "time" - "golang.org/x/net/internal/quic/qlog" + "golang.org/x/net/quic/qlog" ) func TestConnect(t *testing.T) { diff --git a/internal/quic/errors.go b/quic/errors.go similarity index 100% rename from internal/quic/errors.go rename to quic/errors.go diff --git a/internal/quic/files_test.go b/quic/files_test.go similarity index 100% rename from internal/quic/files_test.go rename to quic/files_test.go diff --git a/internal/quic/frame_debug.go b/quic/frame_debug.go similarity index 100% rename from internal/quic/frame_debug.go rename to quic/frame_debug.go diff --git a/internal/quic/gate.go b/quic/gate.go similarity index 100% rename from internal/quic/gate.go rename to quic/gate.go diff --git a/internal/quic/gate_test.go b/quic/gate_test.go similarity index 100% rename from internal/quic/gate_test.go rename to quic/gate_test.go diff --git a/internal/quic/gotraceback_test.go b/quic/gotraceback_test.go similarity index 100% rename from internal/quic/gotraceback_test.go rename to quic/gotraceback_test.go diff --git a/internal/quic/idle.go b/quic/idle.go similarity index 100% rename from internal/quic/idle.go rename to quic/idle.go diff --git a/internal/quic/idle_test.go b/quic/idle_test.go similarity index 100% rename from internal/quic/idle_test.go rename to quic/idle_test.go diff --git a/internal/quic/key_update_test.go b/quic/key_update_test.go similarity index 100% rename from internal/quic/key_update_test.go rename to quic/key_update_test.go diff --git a/internal/quic/log.go b/quic/log.go similarity index 100% rename from internal/quic/log.go rename to quic/log.go diff --git a/internal/quic/loss.go b/quic/loss.go similarity index 100% rename from internal/quic/loss.go rename to quic/loss.go diff --git a/internal/quic/loss_test.go b/quic/loss_test.go similarity index 100% rename from internal/quic/loss_test.go rename to quic/loss_test.go diff --git a/internal/quic/main_test.go b/quic/main_test.go similarity index 100% rename from internal/quic/main_test.go rename to quic/main_test.go diff --git a/internal/quic/math.go b/quic/math.go similarity index 100% rename from internal/quic/math.go rename to quic/math.go diff --git a/internal/quic/pacer.go b/quic/pacer.go similarity index 100% rename from internal/quic/pacer.go rename to quic/pacer.go diff --git a/internal/quic/pacer_test.go b/quic/pacer_test.go similarity index 100% rename from internal/quic/pacer_test.go rename to quic/pacer_test.go diff --git a/internal/quic/packet.go b/quic/packet.go similarity index 100% rename from internal/quic/packet.go rename to quic/packet.go diff --git a/internal/quic/packet_codec_test.go b/quic/packet_codec_test.go similarity index 99% rename from internal/quic/packet_codec_test.go rename to quic/packet_codec_test.go index 98b3bbb05..3b39795ef 100644 --- a/internal/quic/packet_codec_test.go +++ b/quic/packet_codec_test.go @@ -15,7 +15,7 @@ import ( "testing" "time" - "golang.org/x/net/internal/quic/qlog" + "golang.org/x/net/quic/qlog" ) func TestParseLongHeaderPacket(t *testing.T) { diff --git a/internal/quic/packet_number.go b/quic/packet_number.go similarity index 100% rename from internal/quic/packet_number.go rename to quic/packet_number.go diff --git a/internal/quic/packet_number_test.go b/quic/packet_number_test.go similarity index 100% rename from internal/quic/packet_number_test.go rename to quic/packet_number_test.go diff --git a/internal/quic/packet_parser.go b/quic/packet_parser.go similarity index 100% rename from internal/quic/packet_parser.go rename to quic/packet_parser.go diff --git a/internal/quic/packet_protection.go b/quic/packet_protection.go similarity index 100% rename from internal/quic/packet_protection.go rename to quic/packet_protection.go diff --git a/internal/quic/packet_protection_test.go b/quic/packet_protection_test.go similarity index 100% rename from internal/quic/packet_protection_test.go rename to quic/packet_protection_test.go diff --git a/internal/quic/packet_test.go b/quic/packet_test.go similarity index 100% rename from internal/quic/packet_test.go rename to quic/packet_test.go diff --git a/internal/quic/packet_writer.go b/quic/packet_writer.go similarity index 100% rename from internal/quic/packet_writer.go rename to quic/packet_writer.go diff --git a/internal/quic/path.go b/quic/path.go similarity index 100% rename from internal/quic/path.go rename to quic/path.go diff --git a/internal/quic/path_test.go b/quic/path_test.go similarity index 100% rename from internal/quic/path_test.go rename to quic/path_test.go diff --git a/internal/quic/ping.go b/quic/ping.go similarity index 100% rename from internal/quic/ping.go rename to quic/ping.go diff --git a/internal/quic/ping_test.go b/quic/ping_test.go similarity index 100% rename from internal/quic/ping_test.go rename to quic/ping_test.go diff --git a/internal/quic/pipe.go b/quic/pipe.go similarity index 100% rename from internal/quic/pipe.go rename to quic/pipe.go diff --git a/internal/quic/pipe_test.go b/quic/pipe_test.go similarity index 100% rename from internal/quic/pipe_test.go rename to quic/pipe_test.go diff --git a/internal/quic/qlog.go b/quic/qlog.go similarity index 100% rename from internal/quic/qlog.go rename to quic/qlog.go diff --git a/internal/quic/qlog/handler.go b/quic/qlog/handler.go similarity index 100% rename from internal/quic/qlog/handler.go rename to quic/qlog/handler.go diff --git a/internal/quic/qlog/json_writer.go b/quic/qlog/json_writer.go similarity index 100% rename from internal/quic/qlog/json_writer.go rename to quic/qlog/json_writer.go diff --git a/internal/quic/qlog/json_writer_test.go b/quic/qlog/json_writer_test.go similarity index 100% rename from internal/quic/qlog/json_writer_test.go rename to quic/qlog/json_writer_test.go diff --git a/internal/quic/qlog/qlog.go b/quic/qlog/qlog.go similarity index 100% rename from internal/quic/qlog/qlog.go rename to quic/qlog/qlog.go diff --git a/internal/quic/qlog/qlog_test.go b/quic/qlog/qlog_test.go similarity index 100% rename from internal/quic/qlog/qlog_test.go rename to quic/qlog/qlog_test.go diff --git a/internal/quic/qlog_test.go b/quic/qlog_test.go similarity index 99% rename from internal/quic/qlog_test.go rename to quic/qlog_test.go index 6c79c6cf4..c0b5cd170 100644 --- a/internal/quic/qlog_test.go +++ b/quic/qlog_test.go @@ -17,7 +17,7 @@ import ( "testing" "time" - "golang.org/x/net/internal/quic/qlog" + "golang.org/x/net/quic/qlog" ) func TestQLogHandshake(t *testing.T) { diff --git a/internal/quic/queue.go b/quic/queue.go similarity index 100% rename from internal/quic/queue.go rename to quic/queue.go diff --git a/internal/quic/queue_test.go b/quic/queue_test.go similarity index 100% rename from internal/quic/queue_test.go rename to quic/queue_test.go diff --git a/internal/quic/quic.go b/quic/quic.go similarity index 100% rename from internal/quic/quic.go rename to quic/quic.go diff --git a/internal/quic/quic_test.go b/quic/quic_test.go similarity index 100% rename from internal/quic/quic_test.go rename to quic/quic_test.go diff --git a/internal/quic/rangeset.go b/quic/rangeset.go similarity index 100% rename from internal/quic/rangeset.go rename to quic/rangeset.go diff --git a/internal/quic/rangeset_test.go b/quic/rangeset_test.go similarity index 100% rename from internal/quic/rangeset_test.go rename to quic/rangeset_test.go diff --git a/internal/quic/retry.go b/quic/retry.go similarity index 100% rename from internal/quic/retry.go rename to quic/retry.go diff --git a/internal/quic/retry_test.go b/quic/retry_test.go similarity index 100% rename from internal/quic/retry_test.go rename to quic/retry_test.go diff --git a/internal/quic/rtt.go b/quic/rtt.go similarity index 100% rename from internal/quic/rtt.go rename to quic/rtt.go diff --git a/internal/quic/rtt_test.go b/quic/rtt_test.go similarity index 100% rename from internal/quic/rtt_test.go rename to quic/rtt_test.go diff --git a/internal/quic/sent_packet.go b/quic/sent_packet.go similarity index 100% rename from internal/quic/sent_packet.go rename to quic/sent_packet.go diff --git a/internal/quic/sent_packet_list.go b/quic/sent_packet_list.go similarity index 100% rename from internal/quic/sent_packet_list.go rename to quic/sent_packet_list.go diff --git a/internal/quic/sent_packet_list_test.go b/quic/sent_packet_list_test.go similarity index 100% rename from internal/quic/sent_packet_list_test.go rename to quic/sent_packet_list_test.go diff --git a/internal/quic/sent_packet_test.go b/quic/sent_packet_test.go similarity index 100% rename from internal/quic/sent_packet_test.go rename to quic/sent_packet_test.go diff --git a/internal/quic/sent_val.go b/quic/sent_val.go similarity index 100% rename from internal/quic/sent_val.go rename to quic/sent_val.go diff --git a/internal/quic/sent_val_test.go b/quic/sent_val_test.go similarity index 100% rename from internal/quic/sent_val_test.go rename to quic/sent_val_test.go diff --git a/internal/quic/stateless_reset.go b/quic/stateless_reset.go similarity index 100% rename from internal/quic/stateless_reset.go rename to quic/stateless_reset.go diff --git a/internal/quic/stateless_reset_test.go b/quic/stateless_reset_test.go similarity index 100% rename from internal/quic/stateless_reset_test.go rename to quic/stateless_reset_test.go diff --git a/internal/quic/stream.go b/quic/stream.go similarity index 100% rename from internal/quic/stream.go rename to quic/stream.go diff --git a/internal/quic/stream_limits.go b/quic/stream_limits.go similarity index 100% rename from internal/quic/stream_limits.go rename to quic/stream_limits.go diff --git a/internal/quic/stream_limits_test.go b/quic/stream_limits_test.go similarity index 100% rename from internal/quic/stream_limits_test.go rename to quic/stream_limits_test.go diff --git a/internal/quic/stream_test.go b/quic/stream_test.go similarity index 100% rename from internal/quic/stream_test.go rename to quic/stream_test.go diff --git a/internal/quic/tls.go b/quic/tls.go similarity index 100% rename from internal/quic/tls.go rename to quic/tls.go diff --git a/internal/quic/tls_test.go b/quic/tls_test.go similarity index 100% rename from internal/quic/tls_test.go rename to quic/tls_test.go diff --git a/internal/quic/tlsconfig_test.go b/quic/tlsconfig_test.go similarity index 100% rename from internal/quic/tlsconfig_test.go rename to quic/tlsconfig_test.go diff --git a/internal/quic/transport_params.go b/quic/transport_params.go similarity index 100% rename from internal/quic/transport_params.go rename to quic/transport_params.go diff --git a/internal/quic/transport_params_test.go b/quic/transport_params_test.go similarity index 100% rename from internal/quic/transport_params_test.go rename to quic/transport_params_test.go diff --git a/internal/quic/udp.go b/quic/udp.go similarity index 100% rename from internal/quic/udp.go rename to quic/udp.go diff --git a/internal/quic/udp_darwin.go b/quic/udp_darwin.go similarity index 100% rename from internal/quic/udp_darwin.go rename to quic/udp_darwin.go diff --git a/internal/quic/udp_linux.go b/quic/udp_linux.go similarity index 100% rename from internal/quic/udp_linux.go rename to quic/udp_linux.go diff --git a/internal/quic/udp_msg.go b/quic/udp_msg.go similarity index 100% rename from internal/quic/udp_msg.go rename to quic/udp_msg.go diff --git a/internal/quic/udp_other.go b/quic/udp_other.go similarity index 100% rename from internal/quic/udp_other.go rename to quic/udp_other.go diff --git a/internal/quic/udp_test.go b/quic/udp_test.go similarity index 100% rename from internal/quic/udp_test.go rename to quic/udp_test.go diff --git a/internal/quic/version_test.go b/quic/version_test.go similarity index 100% rename from internal/quic/version_test.go rename to quic/version_test.go diff --git a/internal/quic/wire.go b/quic/wire.go similarity index 100% rename from internal/quic/wire.go rename to quic/wire.go diff --git a/internal/quic/wire_test.go b/quic/wire_test.go similarity index 100% rename from internal/quic/wire_test.go rename to quic/wire_test.go From 3dfd003ad338913e62ad1e56020aee316f1ffe59 Mon Sep 17 00:00:00 2001 From: Aleksei Besogonov Date: Fri, 12 Jan 2024 07:38:27 +0000 Subject: [PATCH 19/22] websocket: add support for dialing with context Right now there is no way to pass context.Context to websocket.Dial. In addition, this method can block indefinitely in the NewClient call. Fixes golang/go#57953. Change-Id: Ic52d4b8306cd0850e78d683abb1bf11f0d4247ca GitHub-Last-Rev: 5e8c3a7cbaa324d6165ff40d2ee9ea6c4433b036 GitHub-Pull-Request: golang/net#160 Reviewed-on: https://go-review.googlesource.com/c/net/+/463097 Auto-Submit: Damien Neil Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov --- websocket/client.go | 56 +++++++++++++++++++++++++++++++++--------- websocket/dial.go | 11 ++++++--- websocket/dial_test.go | 37 ++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 15 deletions(-) diff --git a/websocket/client.go b/websocket/client.go index 69a4ac7ee..2c737f77a 100644 --- a/websocket/client.go +++ b/websocket/client.go @@ -6,10 +6,12 @@ package websocket import ( "bufio" + "context" "io" "net" "net/http" "net/url" + "time" ) // DialError is an error that occurs while dialling a websocket server. @@ -77,30 +79,60 @@ func parseAuthority(location *url.URL) string { return location.Host } -// DialConfig opens a new client connection to a WebSocket with a config. func DialConfig(config *Config) (ws *Conn, err error) { - var client net.Conn + return config.DialContext(context.Background()) +} + +// DialContext opens a new client connection to a WebSocket, with context support for timeouts/cancellation. +func (config *Config) DialContext(ctx context.Context) (*Conn, error) { if config.Location == nil { return nil, &DialError{config, ErrBadWebSocketLocation} } if config.Origin == nil { return nil, &DialError{config, ErrBadWebSocketOrigin} } + dialer := config.Dialer if dialer == nil { dialer = &net.Dialer{} } - client, err = dialWithDialer(dialer, config) - if err != nil { - goto Error - } - ws, err = NewClient(config, client) + + client, err := dialWithDialer(ctx, dialer, config) if err != nil { - client.Close() - goto Error + return nil, &DialError{config, err} } - return -Error: - return nil, &DialError{config, err} + // Cleanup the connection if we fail to create the websocket successfully + success := false + defer func() { + if !success { + _ = client.Close() + } + }() + + var ws *Conn + var wsErr error + doneConnecting := make(chan struct{}) + go func() { + defer close(doneConnecting) + ws, err = NewClient(config, client) + if err != nil { + wsErr = &DialError{config, err} + } + }() + + // The websocket.NewClient() function can block indefinitely, make sure that we + // respect the deadlines specified by the context. + select { + case <-ctx.Done(): + // Force the pending operations to fail, terminating the pending connection attempt + _ = client.SetDeadline(time.Now()) + <-doneConnecting // Wait for the goroutine that tries to establish the connection to finish + return nil, &DialError{config, ctx.Err()} + case <-doneConnecting: + if wsErr == nil { + success = true // Disarm the deferred connection cleanup + } + return ws, wsErr + } } diff --git a/websocket/dial.go b/websocket/dial.go index 2dab943a4..8a2d83c47 100644 --- a/websocket/dial.go +++ b/websocket/dial.go @@ -5,18 +5,23 @@ package websocket import ( + "context" "crypto/tls" "net" ) -func dialWithDialer(dialer *net.Dialer, config *Config) (conn net.Conn, err error) { +func dialWithDialer(ctx context.Context, dialer *net.Dialer, config *Config) (conn net.Conn, err error) { switch config.Location.Scheme { case "ws": - conn, err = dialer.Dial("tcp", parseAuthority(config.Location)) + conn, err = dialer.DialContext(ctx, "tcp", parseAuthority(config.Location)) case "wss": - conn, err = tls.DialWithDialer(dialer, "tcp", parseAuthority(config.Location), config.TlsConfig) + tlsDialer := &tls.Dialer{ + NetDialer: dialer, + Config: config.TlsConfig, + } + conn, err = tlsDialer.DialContext(ctx, "tcp", parseAuthority(config.Location)) default: err = ErrBadScheme } diff --git a/websocket/dial_test.go b/websocket/dial_test.go index aa03e30dd..dd844872c 100644 --- a/websocket/dial_test.go +++ b/websocket/dial_test.go @@ -5,10 +5,13 @@ package websocket import ( + "context" "crypto/tls" + "errors" "fmt" "log" "net" + "net/http" "net/http/httptest" "testing" "time" @@ -41,3 +44,37 @@ func TestDialConfigTLSWithDialer(t *testing.T) { t.Fatalf("expected timeout error, got %#v", neterr) } } + +func TestDialConfigTLSWithTimeouts(t *testing.T) { + t.Parallel() + + finishedRequest := make(chan bool) + + // Context for cancellation + ctx, cancel := context.WithCancel(context.Background()) + + // This is a TLS server that blocks each request indefinitely (and cancels the context) + tlsServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cancel() + <-finishedRequest + })) + + tlsServerAddr := tlsServer.Listener.Addr().String() + log.Print("Test TLS WebSocket server listening on ", tlsServerAddr) + defer tlsServer.Close() + defer close(finishedRequest) + + config, _ := NewConfig(fmt.Sprintf("wss://%s/echo", tlsServerAddr), "http://localhost") + config.TlsConfig = &tls.Config{ + InsecureSkipVerify: true, + } + + _, err := config.DialContext(ctx) + dialerr, ok := err.(*DialError) + if !ok { + t.Fatalf("DialError expected, got %#v", err) + } + if !errors.Is(dialerr.Err, context.Canceled) { + t.Fatalf("context.Canceled error expected, got %#v", dialerr.Err) + } +} From 9fb4a8c9216d09f29d58e45a79cc2065d1b5bbf5 Mon Sep 17 00:00:00 2001 From: bestgopher <84328409@qq.com> Date: Tue, 6 Feb 2024 03:08:04 +0000 Subject: [PATCH 20/22] http2: send an error of FLOW_CONTROL_ERROR when exceed the maximum octets According to rfc9113 "https://www.rfc-editor.org/rfc/rfc9113.html#section-6.9.1-7", if a sender receives a WINDOW_UPDATE that causes a flow-control window to exceed this maximum, it MUST terminate either the stream or the connection, as appropriate. For streams, the sender sends a RST_STREAM with an error code of FLOW_CONTROL_ERROR. Change-Id: I5e14db247012ebc860a23053f73e70b83c7cd85d GitHub-Last-Rev: d1a85d3381f634904fc292c9c0a920dd1341adfd GitHub-Pull-Request: golang/net#204 Reviewed-on: https://go-review.googlesource.com/c/net/+/561035 Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Carlos Amedee Reviewed-by: Damien Neil --- http2/transport.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/http2/transport.go b/http2/transport.go index df578b86c..c2a5b44b3 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -2911,6 +2911,15 @@ func (rl *clientConnReadLoop) processWindowUpdate(f *WindowUpdateFrame) error { fl = &cs.flow } if !fl.add(int32(f.Increment)) { + // For stream, the sender sends RST_STREAM with an error code of FLOW_CONTROL_ERROR + if cs != nil { + rl.endStreamError(cs, StreamError{ + StreamID: f.StreamID, + Code: ErrCodeFlowControl, + }) + return nil + } + return ConnectionError(ErrCodeFlowControl) } cc.cond.Broadcast() From c289c7ab4f437bb502e685daaae72426126d5595 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Sat, 2 Mar 2024 18:37:38 -0500 Subject: [PATCH 21/22] websocket: re-add documentation for DialConfig The comment of the DialConfig function was dropped during CL 463097. There doesn't seem to be a good reason to do that, so bring it back. For golang/go#57953. Change-Id: I3e458b7d18cdab95763f003da5a644c8287b54ad Reviewed-on: https://go-review.googlesource.com/c/net/+/568198 Reviewed-by: Dmitri Shuralyov LUCI-TryBot-Result: Go LUCI Reviewed-by: Damien Neil Auto-Submit: Dmitri Shuralyov --- websocket/client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/websocket/client.go b/websocket/client.go index 2c737f77a..1e64157f3 100644 --- a/websocket/client.go +++ b/websocket/client.go @@ -79,6 +79,7 @@ func parseAuthority(location *url.URL) string { return location.Host } +// DialConfig opens a new client connection to a WebSocket with a config. func DialConfig(config *Config) (ws *Conn, err error) { return config.DialContext(context.Background()) } From 7ee34a078aecd23a99f205bded144e5246a27d7c Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Mon, 4 Mar 2024 18:45:30 +0000 Subject: [PATCH 22/22] go.mod: update golang.org/x dependencies Update golang.org/x dependencies to their latest tagged versions. Change-Id: I6d2aa8edee71b255fb6970eb5d817a20df7cc357 Reviewed-on: https://go-review.googlesource.com/c/net/+/568895 Auto-Submit: Gopher Robot Reviewed-by: Than McIntosh LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Knyszek --- go.mod | 6 +++--- go.sum | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 7f512d703..36207106d 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module golang.org/x/net go 1.18 require ( - golang.org/x/crypto v0.19.0 - golang.org/x/sys v0.17.0 - golang.org/x/term v0.17.0 + golang.org/x/crypto v0.21.0 + golang.org/x/sys v0.18.0 + golang.org/x/term v0.18.0 golang.org/x/text v0.14.0 ) diff --git a/go.sum b/go.sum index 683b469d6..69fb10498 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,8 @@ -golang.org/x/crypto v0.19.0 h1:ENy+Az/9Y1vSrlrvBSyna3PITt4tiZLf7sgCjZBX7Wo= -golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= -golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= -golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/term v0.17.0 h1:mkTF7LCd6WGJNL3K1Ad7kwxNfYAW6a8a8QqtMblp/4U= -golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= +golang.org/x/crypto v0.21.0 h1:X31++rzVUdKhX5sWmSOFZxx8UW/ldWx55cbf08iNAMA= +golang.org/x/crypto v0.21.0/go.mod h1:0BP7YvVV9gBbVKyeTG0Gyn+gZm94bibOW5BjDEYAOMs= +golang.org/x/sys v0.18.0 h1:DBdB3niSjOA/O0blCZBqDefyWNYveAYMNF1Wum0DYQ4= +golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/term v0.18.0 h1:FcHjZXDMxI8mM3nwhX9HlKop4C0YQvCVCdwYl2wOtE8= +golang.org/x/term v0.18.0/go.mod h1:ILwASektA3OnRv7amZ1xhE/KTR+u50pbXfZ03+6Nx58= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=