Skip to content

Commit

Permalink
Fix Ping data-race
Browse files Browse the repository at this point in the history
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/[email protected]/connection.go:615 +0x3e
  github.com/moby/spdystream.(*Connection).frameHandler()
      /go/pkg/mod/github.com/moby/[email protected]/connection.go:432 +0x144
  github.com/moby/spdystream.(*Connection).Serve.func2()
      /go/pkg/mod/github.com/moby/[email protected]/connection.go:331 +0xa6
  github.com/moby/spdystream.(*Connection).Serve.func4()
      /go/pkg/mod/github.com/moby/[email protected]/connection.go:332 +0x47

Previous write at 0x00c012a966a8 by goroutine 483918:
  github.com/moby/spdystream.(*Connection).Ping()
      /go/pkg/mod/github.com/moby/[email protected]/connection.go:281 +0x117
  github.com/moby/spdystream.(*Connection).Ping-fm()
      <autogenerated>:1 +0x39
  k8s.io/apimachinery/pkg/util/httpstream/spdy.(*connection).sendPings()
      /go/pkg/mod/k8s.io/[email protected]/pkg/util/httpstream/spdy/connection.go:197 +0x1b6
  k8s.io/apimachinery/pkg/util/httpstream/spdy.newConnection.func2()
      /go/pkg/mod/k8s.io/[email protected]/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/[email protected]/connection.go:327 +0x9e
  k8s.io/apimachinery/pkg/util/httpstream/spdy.newConnection.func1()
      /go/pkg/mod/k8s.io/[email protected]/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/[email protected]/pkg/util/httpstream/spdy/connection.go:96 +0x34d
  k8s.io/apimachinery/pkg/util/httpstream/spdy.NewClientConnectionWithPings()
      /go/pkg/mod/k8s.io/[email protected]/pkg/util/httpstream/spdy/connection.go:57 +0x224
  k8s.io/apimachinery/pkg/util/httpstream/spdy.(*SpdyRoundTripper).NewConnection()
      /go/pkg/mod/k8s.io/[email protected]/pkg/util/httpstream/spdy/roundtripper.go:357 +0x644
  k8s.io/client-go/transport/spdy.Negotiate()
      /go/pkg/mod/k8s.io/[email protected]/transport/spdy/spdy.go:98 +0x42d
  k8s.io/client-go/tools/remotecommand.(*streamExecutor).newConnectionAndStream()
      /go/pkg/mod/k8s.io/[email protected]/tools/remotecommand/remotecommand.go:125 +0x2d7
  k8s.io/client-go/tools/remotecommand.(*streamExecutor).StreamWithContext()
      /go/pkg/mod/k8s.io/[email protected]/tools/remotecommand/remotecommand.go:157 +0xbc
==================
```

This PR aims to fix the present data race and enables the race detector
for tests.

Signed-off-by: Tiago Silva <[email protected]>
  • Loading branch information
tigrato committed May 6, 2023
1 parent 57d1ca2 commit ed81dbd
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 9 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,4 @@ jobs:
go install github.com/kunalkushwaha/ltag@latest \
&& ./scripts/validate/fileheader
- name: Test
run: go test -v ./...
run: go test -v -race ./...
25 changes: 17 additions & 8 deletions connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit ed81dbd

Please sign in to comment.