Fix Ping data-race by tigrato · Pull Request #91 · moby/spdystream

tigrato · 2023-05-06T14:10:58Z

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()
      <autogenerated>: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

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() <autogenerated>: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 <tiago.silva@goteleport.com>

tigrato · 2023-05-08T09:24:07Z

@thaJeztah @dims PTAL

tigrato · 2023-07-22T09:21:19Z

@thaJeztah @dims any update on this one?

bertoldia · 2024-03-19T21:18:35Z

Howdy @dmcgowan @tigrato @thaJeztah 👋 Is there any reason this PR has not been merged yet? It's been approved for over 6 months. The bug is impacting other systems (e.g. https://gitlab.com/gitlab-org/gitlab-runner/-/issues/31077). What's missing to get this merged? 🙇

tigrato · 2024-03-19T22:00:17Z

Howdy @dmcgowan @tigrato @thaJeztah 👋 Is there any reason this PR has not been merged yet? It's been approved for over 6 months. The bug is impacting other systems (e.g. https://gitlab.com/gitlab-org/gitlab-runner/-/issues/31077). What's missing to get this merged? 🙇

I am waiting for someone with write access to merge the PR.

At gravitational, we keep a fork of this library https://github.com/gravitational/spdystream (branch is teleport) where we included this fix to avoid having crashes and several data races flagged during tests.

bertoldia · 2024-03-19T22:38:31Z

Thanks @tigrato 👍

puertomontt · 2024-06-21T10:57:43Z

ran into this as well. Is this repo dead?

dmcgowan · 2024-06-21T13:29:24Z

This project is open source, feel free to add reviews. Especially if you have a fork and verified the change, thats valuable for determining the change is good. This project is very inactive so its hard to be confident a change is ready to merge. I prefer waiting until multiple reviews to hit the merge button.

Over in https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/4685 we switches to using a fork of spdystream because there was an unfixed bug in upstream that caused a data race in the `Ping` API. The fork with the fix has now been merged back into upstream now (moby/spdystream#91), so we can go back to using upstream.

this PR drops the go.mod replace directive from `github.com/moby/spdystream` now that moby/spdystream#91 was finally merged. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>

* drop internal `spdystream` fork this PR drops the go.mod replace directive from `github.com/moby/spdystream` now that moby/spdystream#91 was finally merged. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com> * run gomod tidy on every module --------- Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>

tigrato force-pushed the fix-ping-data-race branch from 01643f3 to ed81dbd Compare May 6, 2023 14:11

tigrato force-pushed the fix-ping-data-race branch from ed81dbd to 3473c0b Compare May 6, 2023 14:13

tigrato mentioned this pull request May 6, 2023

Restore Kubernetes Integration tests gravitational/teleport#25624

Merged

dmcgowan approved these changes Aug 22, 2023

View reviewed changes

puertomontt mentioned this pull request Jun 25, 2024

data race in spdystream dependency kubernetes/kubernetes#125707

Closed

dmcgowan merged commit 06348fb into moby:master Jun 21, 2024

tigrato mentioned this pull request Jun 26, 2024

drop internal spdystream fork gravitational/teleport#43557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Ping data-race#91

Fix Ping data-race#91
dmcgowan merged 1 commit into
moby:masterfrom
tigrato:fix-ping-data-race

tigrato commented May 6, 2023 •

edited

Loading

Uh oh!

tigrato commented May 8, 2023

Uh oh!

tigrato commented Jul 22, 2023

Uh oh!

bertoldia commented Mar 19, 2024

Uh oh!

tigrato commented Mar 19, 2024

Uh oh!

bertoldia commented Mar 19, 2024

Uh oh!

puertomontt commented Jun 21, 2024

Uh oh!

dmcgowan commented Jun 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

tigrato commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tigrato commented May 8, 2023

Uh oh!

tigrato commented Jul 22, 2023

Uh oh!

bertoldia commented Mar 19, 2024

Uh oh!

tigrato commented Mar 19, 2024

Uh oh!

bertoldia commented Mar 19, 2024

Uh oh!

puertomontt commented Jun 21, 2024

Uh oh!

dmcgowan commented Jun 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tigrato commented May 6, 2023 •

edited

Loading