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

DATA RACE when using ghttp.AllowUnhandledRequests #173

Closed
jhvhs opened this issue Sep 15, 2016 · 4 comments
Closed

DATA RACE when using ghttp.AllowUnhandledRequests #173

jhvhs opened this issue Sep 15, 2016 · 4 comments
Assignees
@kirederik @edwardecook
Labels
enhancement good first issue

Comments

@jhvhs
Copy link

jhvhs commented Sep 15, 2016

When running tests with ginkgo -r -race -skipMeasurements -randomizeAllSpecs
and the ghttp.Server is configured as:

  server := ghttp.NewServer()
  server.AllowUnhandledRequests = true
  server.UnhandledRequestStatusCode = 200

The tests running against the server pass, but the output is cluttered with the DATA RACE stack trace:

WARNING: DATA RACE
Read at 0x00c4201e2008 by goroutine 16:
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/gomega/ghttp.(*Server).ServeHTTP()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/gomega/ghttp/test_server.go:261 +0x662
  net/http.serverHandler.ServeHTTP()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/server.go:2202 +0xbb
  net/http.(*conn).serve()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/server.go:1579 +0x5f6

Previous write at 0x00c4201e2008 by goroutine 8:
  github.com/pivotal-cf/dse_opscenter_superuser_updater_test.glob..func3.1.1()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/main_test.go:21 +0xb8
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:109 +0xbb
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:63 +0x141
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*SetupNode).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes/setup_nodes.go:14 +0x88
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).runSample()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:158 +0x27f
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:127 +0x109
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpec()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:172 +0x163
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpecs()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:142 +0x4c7
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:61 +0x11c
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/suite.(*Suite).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/suite/suite.go:59 +0x35d
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo.RunSpecsWithCustomReporters()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:210 +0x35f
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo.RunSpecs()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:191 +0x2b9
  github.com/pivotal-cf/dse_opscenter_superuser_updater_test.TestDseOpscenterSuperuserUpdater()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/dse_opscenter_superuser_updater_suite_test.go:13 +0x88
  testing.tRunner()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:610 +0xc9

Goroutine 16 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/server.go:2293 +0x540
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/httptest/server.go:235 +0xa2

Goroutine 8 (running) created at:
  testing.(*T).Run()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:646 +0x52f
  testing.RunTests.func1()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:793 +0xb9
  testing.tRunner()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:610 +0xc9
  testing.RunTests()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:799 +0x4ba
  testing.(*M).Run()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:743 +0x12f
  main.main()
      github.com/pivotal-cf/dse_opscenter_superuser_updater/_test/_testmain.go:56 +0x1b8
==================
==================
WARNING: DATA RACE
Read at 0x00c4201e2010 by goroutine 16:
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/gomega/ghttp.(*Server).ServeHTTP()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/gomega/ghttp/test_server.go:264 +0x718
  net/http.serverHandler.ServeHTTP()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/server.go:2202 +0xbb
  net/http.(*conn).serve()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/server.go:1579 +0x5f6

Previous write at 0x00c4201e2010 by goroutine 8:
  github.com/pivotal-cf/dse_opscenter_superuser_updater_test.glob..func3.1.1()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/main_test.go:22 +0xe5
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:109 +0xbb
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:63 +0x141
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*SetupNode).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/leafnodes/setup_nodes.go:14 +0x88
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).runSample()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:158 +0x27f
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:127 +0x109
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpec()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:172 +0x163
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpecs()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:142 +0x4c7
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:61 +0x11c
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/suite.(*Suite).Run()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/internal/suite/suite.go:59 +0x35d
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo.RunSpecsWithCustomReporters()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:210 +0x35f
  github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo.RunSpecs()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:191 +0x2b9
  github.com/pivotal-cf/dse_opscenter_superuser_updater_test.TestDseOpscenterSuperuserUpdater()
      /Users/pivotal/workspace/cf-dse-cassandra-service-release/src/github.com/pivotal-cf/dse_opscenter_superuser_updater/dse_opscenter_superuser_updater_suite_test.go:13 +0x88
  testing.tRunner()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:610 +0xc9

Goroutine 16 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/server.go:2293 +0x540
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/Cellar/go/1.7.1/libexec/src/net/http/httptest/server.go:235 +0xa2

Goroutine 8 (running) created at:
  testing.(*T).Run()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:646 +0x52f
  testing.RunTests.func1()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:793 +0xb9
  testing.tRunner()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:610 +0xc9
  testing.RunTests()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:799 +0x4ba
  testing.(*M).Run()
      /usr/local/Cellar/go/1.7.1/libexec/src/testing/testing.go:743 +0x12f
  main.main()
      github.com/pivotal-cf/dse_opscenter_superuser_updater/_test/_testmain.go:56 +0x1b8
==================
• SUCCESS! 595.612626ms PASS | FOCUSED
@onsi
Copy link
Owner

onsi commented Sep 15, 2016

Hmm. I'm not sure but the only fix to this might be to add new setters and getters and a new lock in order to lock around AllowUnhandledRequests and UnhandledRequestStatusCode.

We can leave the two properties exported so as not to break backward compatibility but then update the documentation to recommend people use the setters and getters.

Would you be up for submitting a PR for this @jhvhs ?

@onsi
Copy link
Owner

onsi commented Jun 7, 2017

:bump: any thoughts on a PR?

@williammartin
Copy link
Sponsor Collaborator 

var _ = Describe("Ghttprace", func() {

	var server *ghttp.Server
	var wg *sync.WaitGroup

	BeforeEach(func() {
		server = ghttp.NewServer()
		server.UnhandledRequestStatusCode = 421
		wg = &sync.WaitGroup{}
	})

	AfterEach(func() {
		server.Close()
	})

	JustBeforeEach(func() {
		wg.Add(100)
		go func(group *sync.WaitGroup) {
			for i := 0; i < 100; i++ {
				_, err := http.Get(server.URL())
				Expect(err).NotTo(HaveOccurred())
				group.Done()
			}
		}(wg)
	})

	Context("setting up some server stuff", func() {
		It("races", func() {
			server.AllowUnhandledRequests = true
			wg.Wait()
		})
	})
})

This reproduces the racing behaviour here, though doesn't quite match the setup. However I can totally imagine someone writing a test that flows:

create-server
start-doing-http-requests
change-unhandled-request-status-code-to-exhibit-failure

So if we do an enhancement here and add mutexed getters/setters, maybe we can work off the code ^ to test drive it out.

kirederik pushed a commit that referenced this issue Feb 1, 2018
@edwardecook
Use setters and getters to avoid race condition
13057c3 
when accessing ghttp.AllowUnhandledRequests and ghttp.UnhandledRequestsStatusCode

Solves #173

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
@kirederik kirederik mentioned this issue Feb 1, 2018
Merged
@nodo
Copy link
Collaborator

nodo commented Feb 9, 2018

This issue has been fixed, closing 🎉

@nodo nodo closed this as completed Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kirederik kirederik

@edwardecook edwardecook

Labels
enhancement good first issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@kirederik @onsi @nodo @jhvhs @williammartin @edwardecook