Skip to content
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.

Sign up for GitHub

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

Jump to bottom

net/http/fcgi: race detected during execution of TestResponseWriterSniffsContentType test #41167

Closed
dmitshur opened this issue Sep 1, 2020 · 8 comments
Closed

net/http/fcgi: race detected during execution of TestResponseWriterSniffsContentType test #41167

dmitshur opened this issue Sep 1, 2020 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Sep 1, 2020

What version of Go are you using (go version)?

$ go version
go version go1.15.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/dmitshur/Library/Caches/go-build"
GOENV="/Users/dmitshur/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/dmitshur/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/dmitshur/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/zb/5p8cwfhj29gf_m8vdy8ylmlr00jwcj/T/go-build487751365=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I looked at https://build.golang.org/?branch=release-branch.go1.15 (and https://build.golang.org/?branch=release-branch.go1.14) and spotted a few new failures on race builders. Then I reproduced it locally by running:

$ go test -count=10 -run=TestResponseWriterSniffsContentType -race net/http/fcgi

What did you expect to see?

ok  	net/http/fcgi	0.171s

What did you see instead?

[...]
--- FAIL: TestResponseWriterSniffsContentType (0.00s)
    --- FAIL: TestResponseWriterSniffsContentType/jpg (0.00s)
    testing.go:1023: race detected during execution of test
FAIL
FAIL	net/http/fcgi	0.155s

/cc @FiloSottile @empijei
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 1, 2020
@dmitshur dmitshur added this to the Go1.16 milestone Sep 1, 2020
@tz70s
Copy link
Contributor

tz70s commented Sep 2, 2020

Probably a connection lock is needed for this read, https://github.com/golang/go/blob/master/src/net/http/fcgi/child.go#L158

writes from typeStdin are asynchronous. https://github.com/golang/go/blob/master/src/net/http/fcgi/child.go#L229

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/252417 mentions this issue: net/http: fix racy fastcgi on connection read/write

@empijei
Copy link
Contributor

empijei commented Sep 2, 2020

Hi, I have looked into this a bit more and I tried to backport the racy test to 1.15.

The race was already there and was not introduced by the security release.

The only difference is that now we cover that codepath with tests, so I think this race has been there for a long time.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/252717 mentions this issue: [release-branch.go1.15] net/http/fgci: skip flaky test

@dmitshur
Copy link
Contributor Author

dmitshur commented Sep 2, 2020

Sounds good. Since the race isn't new and critical, the fix can be made available in Go 1.16 and we don't need to backport it. However, let's skip the new test so that the race builders continue to pass and provide full coverage on the release branches.

@gopherbot Please backport to both Go 1.15 and 1.14, but only the test fix.

This was referenced Sep 2, 2020
Closed
Closed
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #41192 (for 1.14), #41193 (for 1.15), #41194 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/252718 mentions this issue: [release-branch.go1.14] net/http/fgci: skip flaky test

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 2, 2020
gopherbot pushed a commit that referenced this issue Sep 2, 2020
@FiloSottile @dmitshur
[release-branch.go1.15] net/http/fgci: skip flaky test
33e8600 
A test introduced in the security release is flaky due to a pre-existing
issue that does not qualify for backport itself.

Updates #41167
Fixes #41193

Change-Id: Ie6014e0796c1baee7b077881b5a799f9947fc9c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/252717
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Sep 2, 2020
@FiloSottile @dmitshur
[release-branch.go1.14] net/http/fgci: skip flaky test
532db5d 
A test introduced in the security release is flaky due to a pre-existing
issue that does not qualify for backport itself.

Updates #41167
Fixes #41192

Change-Id: Ie6014e0796c1baee7b077881b5a799f9947fc9c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/252718
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
@FiloSottile @claucece
[release-branch.go1.15] net/http/fgci: skip flaky test

Verified

This commit was signed with the committer’s verified signature. The key has expired.
claucece Sofía Celi
GPG key ID: 311C4B2B2FB51BE8
Expired
Verified
Learn about vigilant mode
ced1366 
A test introduced in the security release is flaky due to a pre-existing
issue that does not qualify for backport itself.

Updates golang#41167
Fixes golang#41193

Change-Id: Ie6014e0796c1baee7b077881b5a799f9947fc9c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/252717
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@hillu hillu mentioned this issue Jan 25, 2021
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275692 mentions this issue: net/http/fcgi: remove locking added to prevent a test-only race

gopherbot pushed a commit that referenced this issue Jan 26, 2021
@hillu @ianlancetaylor
net/http/fcgi: remove locking added to prevent a test-only race
ce8b318 
The race reported in issue #41167 was detected only because the
ReadWriter used in test code happened to be a bytes.Buffer whose
Read and Write operate (unsafely) on shared state. This is not the
case in any realistic scenario where the FastCGI protocol is spoken
over sockets or pairs of pipes.

Since tests that use nopWriteCloser don't care about any output
generate by child.Serve(), we change nopWriteCloser to provide
a dummy Write method.

Remove the locking added in CL 252417, since it causes a deadlock
during write as reported in #43901. The race in tests no longer
happens thanks to the aforementioned change to nopWriteCloser.

Fixes #43901.
Updates #41167.

Change-Id: I8cf31088a71253c34056698f8e2ad0bee9fcf6c6
GitHub-Last-Rev: b06d837
GitHub-Pull-Request: #43027
Reviewed-on: https://go-review.googlesource.com/c/go/+/275692
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Jan 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
4 participants
@empijei @dmitshur @gopherbot @tz70s