Merge pull request #4005 from quic-go/gso-detection

enable GSO, disable if sending fails for a particular address

mockgen.go

@@ -5,6 +5,9 @@ package quic
 //go:generate sh -c "go run github.com/golang/mock/mockgen -build_flags=\"-tags=gomock\" -package quic -self_package github.com/quic-go/quic-go -destination mock_send_conn_test.go github.com/quic-go/quic-go SendConn"
 type SendConn = sendConn
 
+//go:generate sh -c "go run github.com/golang/mock/mockgen -build_flags=\"-tags=gomock\" -package quic -self_package github.com/quic-go/quic-go -destination mock_raw_conn_test.go github.com/quic-go/quic-go RawConn"
+type RawConn = rawConn
+
 //go:generate sh -c "go run github.com/golang/mock/mockgen -build_flags=\"-tags=gomock\" -package quic -self_package github.com/quic-go/quic-go -destination mock_sender_test.go github.com/quic-go/quic-go Sender"
 type Sender = sender
 

packet_handler_map.go

@@ -26,9 +26,9 @@ type connCapabilities struct {
 // rawConn is a connection that allow reading of a receivedPackeh.
 type rawConn interface {
 	ReadPacket() (receivedPacket, error)
-	// The size parameter is used for GSO.
-	// If GSO is not support, len(b) must be equal to size.
-	WritePacket(b []byte, size uint16, addr net.Addr, oob []byte) (int, error)
+	// WritePacket writes a packet on the wire.
+	// If GSO is enabled, it's the caller's responsibility to set the correct control message.
+	WritePacket(b []byte, addr net.Addr, oob []byte) (int, error)
 	LocalAddr() net.Addr
 	SetReadDeadline(time.Time) error
 	io.Closer

send_conn.go

@@ -1,10 +1,12 @@
 package quic
 
 import (
+	"fmt"
 	"math"
 	"net"
 
 	"github.com/quic-go/quic-go/internal/protocol"
+	"github.com/quic-go/quic-go/internal/utils"
 )
 
 // A sendConn allows sending using a simple Write() on a non-connected packet conn.
@@ -20,61 +22,86 @@ type sendConn interface {
 type sconn struct {
 	rawConn
 
+	localAddr  net.Addr
 	remoteAddr net.Addr
-	info       packetInfo
-	oob        []byte
+
+	logger utils.Logger
+
+	info packetInfo
+	oob  []byte
+	// If GSO enabled, and we receive a GSO error for this remote address, GSO is disabled.
+	gotGSOError bool
 }
 
 var _ sendConn = &sconn{}
 
-func newSendConn(c rawConn, remote net.Addr) *sconn {
-	sc := &sconn{
-		rawConn:    c,
-		remoteAddr: remote,
-	}
-	if c.capabilities().GSO {
-		// add 32 bytes, so we can add the UDP_SEGMENT msg
-		sc.oob = make([]byte, 0, 32)
+func newSendConn(c rawConn, remote net.Addr, info packetInfo, logger utils.Logger) *sconn {
+	localAddr := c.LocalAddr()
+	if info.addr.IsValid() {
+		if udpAddr, ok := localAddr.(*net.UDPAddr); ok {
+			addrCopy := *udpAddr
+			addrCopy.IP = info.addr.AsSlice()
+			localAddr = &addrCopy
+		}
 	}
-	return sc
-}
 
-func newSendConnWithPacketInfo(c rawConn, remote net.Addr, info packetInfo) *sconn {
 	oob := info.OOB()
-	if c.capabilities().GSO {
-		// add 32 bytes, so we can add the UDP_SEGMENT msg
-		l := len(oob)
-		oob = append(oob, make([]byte, 32)...)
-		oob = oob[:l]
-	}
+	// add 32 bytes, so we can add the UDP_SEGMENT msg
+	l := len(oob)
+	oob = append(oob, make([]byte, 32)...)
+	oob = oob[:l]
 	return &sconn{
 		rawConn:    c,
+		localAddr:  localAddr,
 		remoteAddr: remote,
 		info:       info,
 		oob:        oob,
+		logger:     logger,
 	}
 }
 
 func (c *sconn) Write(p []byte, size protocol.ByteCount) error {
+	if !c.capabilities().GSO {
+		if protocol.ByteCount(len(p)) != size {
+			panic(fmt.Sprintf("inconsistent packet size (%d vs %d)", len(p), size))
+		}
+		_, err := c.WritePacket(p, c.remoteAddr, c.oob)
+		return err
+	}
+	// GSO is supported. Append the control message and send.
 	if size > math.MaxUint16 {
 		panic("size overflow")
 	}
-	_, err := c.WritePacket(p, uint16(size), c.remoteAddr, c.oob)
+	_, err := c.WritePacket(p, c.remoteAddr, appendUDPSegmentSizeMsg(c.oob, uint16(size)))
+	if err != nil && isGSOError(err) {
+		// disable GSO for future calls
+		c.gotGSOError = true
+		if c.logger.Debug() {
+			c.logger.Debugf("GSO failed when sending to %s", c.remoteAddr)
+		}
+		// send out the packets one by one
+		for len(p) > 0 {
+			l := len(p)
+			if l > int(size) {
+				l = int(size)
+			}
+			if _, err := c.WritePacket(p[:l], c.remoteAddr, c.oob); err != nil {
+				return err
+			}
+			p = p[l:]
+		}
+		return nil
+	}
 	return err
 }
 
-func (c *sconn) RemoteAddr() net.Addr {
-	return c.remoteAddr
-}
-
-func (c *sconn) LocalAddr() net.Addr {
-	addr := c.rawConn.LocalAddr()
-	if c.info.addr.IsValid() {
-		if udpAddr, ok := addr.(*net.UDPAddr); ok {
-			addrCopy := *udpAddr
-			addrCopy.IP = c.info.addr.AsSlice()
-			addr = &addrCopy
-		}
+func (c *sconn) capabilities() connCapabilities {
+	capabilities := c.rawConn.capabilities()
+	if capabilities.GSO {
+		capabilities.GSO = !c.gotGSOError
 	}
-	return addr
+	return capabilities
 }
+
+func (c *sconn) RemoteAddr() net.Addr { return c.remoteAddr }
+func (c *sconn) LocalAddr() net.Addr  { return c.localAddr }

send_conn_test.go

@@ -2,46 +2,81 @@ package quic
 
 import (
 	"net"
+	"net/netip"
 
+	"github.com/quic-go/quic-go/internal/utils"
+
+	"github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("Connection (for sending packets)", func() {
-	var (
-		c          sendConn
-		packetConn *MockPacketConn
-		addr       net.Addr
-	)
-
-	BeforeEach(func() {
-		addr = &net.UDPAddr{IP: net.IPv4(192, 168, 100, 200), Port: 1337}
-		packetConn = NewMockPacketConn(mockCtrl)
-		rawConn, err := wrapConn(packetConn)
-		Expect(err).ToNot(HaveOccurred())
-		c = newSendConnWithPacketInfo(rawConn, addr, packetInfo{})
-	})
+// Only if appendUDPSegmentSizeMsg actually appends a message (and isn't only a stub implementation),
+// GSO is actually supported on this platform.
+var platformSupportsGSO = len(appendUDPSegmentSizeMsg([]byte{}, 1337)) > 0
 
-	It("writes", func() {
-		packetConn.EXPECT().WriteTo([]byte("foobar"), addr)
-		Expect(c.Write([]byte("foobar"), 6)).To(Succeed())
-	})
+var _ = Describe("Connection (for sending packets)", func() {
+	remoteAddr := &net.UDPAddr{IP: net.IPv4(192, 168, 100, 200), Port: 1337}
 
-	It("gets the remote address", func() {
+	It("gets the local and remote addresses", func() {
+		localAddr := &net.UDPAddr{IP: net.IPv4(192, 168, 0, 1), Port: 1234}
+		rawConn := NewMockRawConn(mockCtrl)
+		rawConn.EXPECT().LocalAddr().Return(localAddr)
+		c := newSendConn(rawConn, remoteAddr, packetInfo{}, utils.DefaultLogger)
+		Expect(c.LocalAddr().String()).To(Equal("192.168.0.1:1234"))
 		Expect(c.RemoteAddr().String()).To(Equal("192.168.100.200:1337"))
 	})
 
-	It("gets the local address", func() {
-		addr := &net.UDPAddr{
-			IP:   net.IPv4(192, 168, 0, 1),
-			Port: 1234,
-		}
-		packetConn.EXPECT().LocalAddr().Return(addr)
-		Expect(c.LocalAddr()).To(Equal(addr))
+	It("uses the local address from the packet info", func() {
+		localAddr := &net.UDPAddr{IP: net.IPv4(192, 168, 0, 1), Port: 1234}
+		rawConn := NewMockRawConn(mockCtrl)
+		rawConn.EXPECT().LocalAddr().Return(localAddr)
+		c := newSendConn(rawConn, remoteAddr, packetInfo{addr: netip.AddrFrom4([4]byte{127, 0, 0, 42})}, utils.DefaultLogger)
+		Expect(c.LocalAddr().String()).To(Equal("127.0.0.42:1234"))
 	})
 
-	It("closes", func() {
-		packetConn.EXPECT().Close()
-		Expect(c.Close()).To(Succeed())
-	})
+	if platformSupportsGSO {
+		It("writes with GSO", func() {
+			rawConn := NewMockRawConn(mockCtrl)
+			rawConn.EXPECT().LocalAddr()
+			rawConn.EXPECT().capabilities().Return(connCapabilities{GSO: true}).AnyTimes()
+			c := newSendConn(rawConn, remoteAddr, packetInfo{}, utils.DefaultLogger)
+			rawConn.EXPECT().WritePacket([]byte("foobar"), remoteAddr, gomock.Any()).Do(func(_ []byte, _ net.Addr, oob []byte) {
+				msg := appendUDPSegmentSizeMsg([]byte{}, 3)
+				Expect(oob).To(Equal(msg))
+			})
+			Expect(c.Write([]byte("foobar"), 3)).To(Succeed())
+		})
+
+		It("disables GSO if writing fails", func() {
+			rawConn := NewMockRawConn(mockCtrl)
+			rawConn.EXPECT().LocalAddr()
+			rawConn.EXPECT().capabilities().Return(connCapabilities{GSO: true}).AnyTimes()
+			c := newSendConn(rawConn, remoteAddr, packetInfo{}, utils.DefaultLogger)
+			Expect(c.capabilities().GSO).To(BeTrue())
+			gomock.InOrder(
+				rawConn.EXPECT().WritePacket([]byte("foobar"), remoteAddr, gomock.Any()).DoAndReturn(func(_ []byte, _ net.Addr, oob []byte) (int, error) {
+					msg := appendUDPSegmentSizeMsg([]byte{}, 3)
+					Expect(oob).To(Equal(msg))
+					return 0, errGSO
+				}),
+				rawConn.EXPECT().WritePacket([]byte("foo"), remoteAddr, gomock.Len(0)).Return(3, nil),
+				rawConn.EXPECT().WritePacket([]byte("bar"), remoteAddr, gomock.Len(0)).Return(3, nil),
+			)
+			Expect(c.Write([]byte("foobar"), 3)).To(Succeed())
+			Expect(c.capabilities().GSO).To(BeFalse()) // GSO support is now disabled
+			// make sure we actually enforce that
+			Expect(func() { c.Write([]byte("foobar"), 3) }).To(PanicWith("inconsistent packet size (6 vs 3)"))
+		})
+	} else {
+		It("writes without GSO", func() {
+			remoteAddr := &net.UDPAddr{IP: net.IPv4(192, 168, 100, 200), Port: 1337}
+			rawConn := NewMockRawConn(mockCtrl)
+			rawConn.EXPECT().LocalAddr()
+			rawConn.EXPECT().capabilities()
+			c := newSendConn(rawConn, remoteAddr, packetInfo{}, utils.DefaultLogger)
+			rawConn.EXPECT().WritePacket([]byte("foobar"), remoteAddr, gomock.Len(0))
+			Expect(c.Write([]byte("foobar"), 6)).To(Succeed())
+		})
+	}
 })