From 3473c0b0cd1433c2a0153d57f461f533cd1cd1e6 Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Sat, 6 May 2023 15:04:51 +0100 Subject: [PATCH] Fix Ping data-race While testing some code under `-race` flag, the race detector flagged a data race on `spdystream.Ping` code path. ``` ================== WARNING: DATA RACE Read at 0x00c012a966a8 by goroutine 483923: github.com/moby/spdystream.(*Connection).handlePingFrame() /go/pkg/mod/github.com/moby/spdystream@v0.2.0/connection.go:615 +0x3e github.com/moby/spdystream.(*Connection).frameHandler() /go/pkg/mod/github.com/moby/spdystream@v0.2.0/connection.go:432 +0x144 github.com/moby/spdystream.(*Connection).Serve.func2() /go/pkg/mod/github.com/moby/spdystream@v0.2.0/connection.go:331 +0xa6 github.com/moby/spdystream.(*Connection).Serve.func4() /go/pkg/mod/github.com/moby/spdystream@v0.2.0/connection.go:332 +0x47 Previous write at 0x00c012a966a8 by goroutine 483918: github.com/moby/spdystream.(*Connection).Ping() /go/pkg/mod/github.com/moby/spdystream@v0.2.0/connection.go:281 +0x117 github.com/moby/spdystream.(*Connection).Ping-fm() :1 +0x39 k8s.io/apimachinery/pkg/util/httpstream/spdy.(*connection).sendPings() /go/pkg/mod/k8s.io/apimachinery@v0.26.3/pkg/util/httpstream/spdy/connection.go:197 +0x1b6 k8s.io/apimachinery/pkg/util/httpstream/spdy.newConnection.func2() /go/pkg/mod/k8s.io/apimachinery@v0.26.3/pkg/util/httpstream/spdy/connection.go:96 +0x47 Goroutine 483923 (running) created at: github.com/moby/spdystream.(*Connection).Serve() /go/pkg/mod/github.com/moby/spdystream@v0.2.0/connection.go:327 +0x9e k8s.io/apimachinery/pkg/util/httpstream/spdy.newConnection.func1() /go/pkg/mod/k8s.io/apimachinery@v0.26.3/pkg/util/httpstream/spdy/connection.go:94 +0x47 Goroutine 483918 (running) created at: k8s.io/apimachinery/pkg/util/httpstream/spdy.newConnection() /go/pkg/mod/k8s.io/apimachinery@v0.26.3/pkg/util/httpstream/spdy/connection.go:96 +0x34d k8s.io/apimachinery/pkg/util/httpstream/spdy.NewClientConnectionWithPings() /go/pkg/mod/k8s.io/apimachinery@v0.26.3/pkg/util/httpstream/spdy/connection.go:57 +0x224 k8s.io/apimachinery/pkg/util/httpstream/spdy.(*SpdyRoundTripper).NewConnection() /go/pkg/mod/k8s.io/apimachinery@v0.26.3/pkg/util/httpstream/spdy/roundtripper.go:357 +0x644 k8s.io/client-go/transport/spdy.Negotiate() /go/pkg/mod/k8s.io/client-go@v0.26.3/transport/spdy/spdy.go:98 +0x42d k8s.io/client-go/tools/remotecommand.(*streamExecutor).newConnectionAndStream() /go/pkg/mod/k8s.io/client-go@v0.26.3/tools/remotecommand/remotecommand.go:125 +0x2d7 k8s.io/client-go/tools/remotecommand.(*streamExecutor).StreamWithContext() /go/pkg/mod/k8s.io/client-go@v0.26.3/tools/remotecommand/remotecommand.go:157 +0xbc ================== ``` This PR aims to fix the present data race. Signed-off-by: Tiago Silva --- connection.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/connection.go b/connection.go index d906bb0..25e4ac2 100644 --- a/connection.go +++ b/connection.go @@ -208,9 +208,10 @@ type Connection struct { nextStreamId spdy.StreamId receivedStreamId spdy.StreamId - pingIdLock sync.Mutex - pingId uint32 - pingChans map[uint32]chan error + // pingLock protects pingChans and pingId + pingLock sync.Mutex + pingId uint32 + pingChans map[uint32]chan error shutdownLock sync.Mutex shutdownChan chan error @@ -274,16 +275,20 @@ func NewConnection(conn net.Conn, server bool) (*Connection, error) { // returns the response time func (s *Connection) Ping() (time.Duration, error) { pid := s.pingId - s.pingIdLock.Lock() + s.pingLock.Lock() if s.pingId > 0x7ffffffe { s.pingId = s.pingId - 0x7ffffffe } else { s.pingId = s.pingId + 2 } - s.pingIdLock.Unlock() pingChan := make(chan error) s.pingChans[pid] = pingChan - defer delete(s.pingChans, pid) + s.pingLock.Unlock() + defer func() { + s.pingLock.Lock() + delete(s.pingChans, pid) + s.pingLock.Unlock() + }() frame := &spdy.PingFrame{Id: pid} startTime := time.Now() @@ -612,10 +617,14 @@ func (s *Connection) handleDataFrame(frame *spdy.DataFrame) error { } func (s *Connection) handlePingFrame(frame *spdy.PingFrame) error { - if s.pingId&0x01 != frame.Id&0x01 { + s.pingLock.Lock() + pingId := s.pingId + pingChan, pingOk := s.pingChans[frame.Id] + s.pingLock.Unlock() + + if pingId&0x01 != frame.Id&0x01 { return s.framer.WriteFrame(frame) } - pingChan, pingOk := s.pingChans[frame.Id] if pingOk { close(pingChan) }