New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Ping data-race #91
base: master
Are you sure you want to change the base?
Conversation
01643f3
to
ed81dbd
Compare
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>
ed81dbd
to
3473c0b
Compare
@thaJeztah @dims PTAL |
@thaJeztah @dims any update on this one? |
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. |
Thanks @tigrato 👍 |
There is currently a race condition in github.com/moby/spdystream library used indirectly in GitLab Runner. There is an existing PR moby/spdystream#91 to have it fixed but it has not been merged yet. In this MR, we use the following fork github.com/gravitational/spdystream where the PR has been merged. The replace go.mod rule can still be removed when the PR mentioned is merged in the moby/spdystream library
While testing some code under
-race
flag, the race detector flagged a data race onspdystream.Ping
code path.This PR aims to fix the present data race