From ce8b318624adcdd45ecd53b33f6bae38bcccc7be Mon Sep 17 00:00:00 2001 From: Hilko Bengen Date: Mon, 25 Jan 2021 22:54:20 +0000 Subject: [PATCH] net/http/fcgi: remove locking added to prevent a test-only race 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: b06d8377fdada075775d79a20577d38a7c471b45 GitHub-Pull-Request: golang/go#43027 Reviewed-on: https://go-review.googlesource.com/c/go/+/275692 Reviewed-by: Ian Lance Taylor Trust: Dmitri Shuralyov --- src/net/http/fcgi/child.go | 3 --- src/net/http/fcgi/fcgi_test.go | 12 ++++++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/net/http/fcgi/child.go b/src/net/http/fcgi/child.go index e97b8440e1..756722ba14 100644 --- a/src/net/http/fcgi/child.go +++ b/src/net/http/fcgi/child.go @@ -171,12 +171,9 @@ func (c *child) serve() { defer c.cleanUp() var rec record for { - c.conn.mutex.Lock() if err := rec.read(c.conn.rwc); err != nil { - c.conn.mutex.Unlock() return } - c.conn.mutex.Unlock() if err := c.handleRecord(&rec); err != nil { return } diff --git a/src/net/http/fcgi/fcgi_test.go b/src/net/http/fcgi/fcgi_test.go index d3b704f821..b58111de20 100644 --- a/src/net/http/fcgi/fcgi_test.go +++ b/src/net/http/fcgi/fcgi_test.go @@ -221,7 +221,11 @@ var cleanUpTests = []struct { } type nopWriteCloser struct { - io.ReadWriter + io.Reader +} + +func (nopWriteCloser) Write(buf []byte) (int, error) { + return len(buf), nil } func (nopWriteCloser) Close() error { @@ -235,7 +239,7 @@ func TestChildServeCleansUp(t *testing.T) { for _, tt := range cleanUpTests { input := make([]byte, len(tt.input)) copy(input, tt.input) - rc := nopWriteCloser{bytes.NewBuffer(input)} + rc := nopWriteCloser{bytes.NewReader(input)} done := make(chan bool) c := newChild(rc, http.HandlerFunc(func( w http.ResponseWriter, @@ -325,7 +329,7 @@ func TestChildServeReadsEnvVars(t *testing.T) { for _, tt := range envVarTests { input := make([]byte, len(tt.input)) copy(input, tt.input) - rc := nopWriteCloser{bytes.NewBuffer(input)} + rc := nopWriteCloser{bytes.NewReader(input)} done := make(chan bool) c := newChild(rc, http.HandlerFunc(func( w http.ResponseWriter, @@ -375,7 +379,7 @@ func TestResponseWriterSniffsContentType(t *testing.T) { t.Run(tt.name, func(t *testing.T) { input := make([]byte, len(streamFullRequestStdin)) copy(input, streamFullRequestStdin) - rc := nopWriteCloser{bytes.NewBuffer(input)} + rc := nopWriteCloser{bytes.NewReader(input)} done := make(chan bool) var resp *response c := newChild(rc, http.HandlerFunc(func( -- 2.50.0