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.

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 after update to Gomega 1.14.0 #457

Closed
blgm opened this issue Jul 12, 2021 · 6 comments
Closed

Data race after update to Gomega 1.14.0 #457

blgm opened this issue Jul 12, 2021 · 6 comments

Comments

@blgm
Copy link
Collaborator

blgm commented Jul 12, 2021

Worked on Gomega 1.13.0. Data race on update to Gomega 1.14.0, which was "fixed" by downgrading.

The code that encountered the data race was: https://github.com/pivotal-cf/on-demand-service-broker/blob/8db9babdd6b9b533fd4f258d075a6230fb83e681/system_tests/test_helpers/cf_helpers/service.go#L149

It's likely that the reason is this new feature that allows assertions to run in Eventually() functions. The code that failed has been doing this (probably incorrectly) for a long time.

Log:

WARNING: DATA RACE
Read at 0x00c000146558 by goroutine 57:
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).pollActual.func1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:87 +0x84
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).pollActual()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:106 +0x17e
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).match()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:149 +0xe4
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).Should()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:52 +0xc6
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.awaitServiceOperation()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:185 +0x1b3
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.AwaitServiceCreationWithTimeout()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:115 +0x184
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.AwaitServiceCreation()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:111 +0x293
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.CreateService()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:34 +0x266
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all.CreateServiceAndApp()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/helpers.go:40 +0x178
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled_test.glob..func1.1.1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled/feature_toggled_test.go:75 +0xaf
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all.PerformInParallel.func1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/helpers.go:29 +0xe6

Previous write at 0x00c000146558 by goroutine 58:
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).pollActual.func1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:88 +0x135
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).pollActual()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:106 +0x17e
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).match()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:149 +0xe4
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion.(*AsyncAssertion).Should()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/gomega/internal/asyncassertion/async_assertion.go:52 +0xc6
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.awaitServiceOperation()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:185 +0x1b3
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.AwaitServiceCreationWithTimeout()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:115 +0x184
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.AwaitServiceCreation()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:111 +0x293
  github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers.CreateService()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/test_helpers/cf_helpers/service.go:34 +0x266
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all.CreateServiceAndApp()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/helpers.go:40 +0x178
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled_test.glob..func1.1.1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled/feature_toggled_test.go:75 +0xaf
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all.PerformInParallel.func1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/helpers.go:29 +0xe6

Goroutine 57 (running) created at:
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all.PerformInParallel()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/helpers.go:26 +0xc8
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled_test.glob..func1.1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled/feature_toggled_test.go:74 +0x6dd
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113 +0xfc
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:64 +0x184
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*SetupNode).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes/setup_nodes.go:15 +0xb9
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).runSample()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:193 +0x2f2
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:138 +0x187
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpec()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:200 +0x17b
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpecs()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:170 +0x235
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:66 +0x145
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/suite.(*Suite).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/suite/suite.go:79 +0x839
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo.runSpecsWithCustomReporters()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:238 +0x357
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo.RunSpecs()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:213 +0xe4
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled_test.TestFeatureToggled()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled/feature_toggled_suite_test.go:12 +0x108
  testing.tRunner()
      /go/src/testing/testing.go:1193 +0x202

Goroutine 58 (running) created at:
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all.PerformInParallel()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/helpers.go:26 +0xc8
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled_test.glob..func1.1()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled/feature_toggled_test.go:74 +0x6dd
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113 +0xfc
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:64 +0x184
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*SetupNode).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/leafnodes/setup_nodes.go:15 +0xb9
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).runSample()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:193 +0x2f2
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec.(*Spec).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/spec/spec.go:138 +0x187
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpec()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:200 +0x17b
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpecs()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:170 +0x235
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:66 +0x145
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/suite.(*Suite).Run()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/internal/suite/suite.go:79 +0x839
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo.runSpecsWithCustomReporters()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:238 +0x357
  github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo.RunSpecs()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:213 +0xe4
  github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled_test.TestFeatureToggled()
      /tmp/build/d8bec1b0/broker-release/src/github.com/pivotal-cf/on-demand-service-broker/system_tests/upgrade_all/feature_toggled/feature_toggled_suite_test.go:12 +0x108
  testing.tRunner()
      /go/src/testing/testing.go:1193 +0x202

It's not immediately obvious to me why this is happing and which code is incorrect. I'm mainly logging this in case someone else immediately sees the issue, or if someone else finds similar issues.

The impact to my team is low because we can just stay on the older version for a while.
@onsi
Copy link
Owner

onsi commented Jul 13, 2021 •
edited

Thanks @blgm - I think you've uncovered an issue with the new feature... and I may have to end up rolling it back (sigh, globals are terrible).

The new feature works by having Eventually swap out the fail handler while it runs. That way it can intercept failures while it's running. Unfortunately, it has to swap out the global fail handler (every assertion in Gomega is calling the global fail handler). That's all well and good when you only have Eventually operating on the main thread. However as soon as you call Eventually in multiple goroutines, those goroutines will race around the global fail handler.

I'll need to think about it but, sadly, I don't see away around this inherent built-in limitation and will likely have to roll this feature back.

@onsi
Copy link
Owner

onsi commented Jul 13, 2021

I think I'll be able to salvage the feature by requiring the user to provide a function that takes in a Gomega and only rewire the fail handler for that instance of Gomega. I'll give it a try and report back.

blgm added a commit to pivotal-cf/on-demand-services-sdk that referenced this issue Jul 15, 2021
@blgm
Revert "Bump github.com/onsi/gomega from 1.13.0 to 1.14.0 (#80)"
ed3da6a 
Because of onsi/gomega#457

This reverts commit f822e17.
@blgm blgm mentioned this issue Jul 15, 2021
Closed
FelisiaM added a commit to pivotal-cf/on-demand-service-broker that referenced this issue Jul 16, 2021
@FelisiaM
Downgrade gomega to v1.13.0 because of [issue](onsi/gomega#457)
484bb94
servicesenablement added a commit to pivotal-cf/on-demand-service-broker-release that referenced this issue Jul 16, 2021
@servicesenablement
[odb] Downgrade gomega to v1.13.0 because of [issue](onsi/gomega#457)
7e25dd5
FelisiaM added a commit to pivotal-cf/on-demand-service-broker that referenced this issue Jul 16, 2021
@FelisiaM
cont: Downgrade gomega to v1.13.0 because of [issue](onsi/gomega#457)
43a4e9b
servicesenablement added a commit to pivotal-cf/on-demand-service-broker-release that referenced this issue Jul 16, 2021
@servicesenablement
[odb] cont: Downgrade gomega to v1.13.0 because of [issue](onsi/gomeg…
f57b489 
…a#457)
@monkey92t monkey92t mentioned this issue Jul 22, 2021
Merged
@onsi onsi mentioned this issue Jul 29, 2021
Closed
@onsi onsi closed this as completed in e60e915 Aug 5, 2021
@onsi onsi reopened this Aug 5, 2021
@onsi
Copy link
Owner

onsi commented Aug 5, 2021

Just shipped 1.15.0 which should fix this issue. PTAL.

@blgm
Copy link
Collaborator Author

blgm commented Aug 6, 2021

Hi @onsi, I've updated to 1.15.0, and I don't see the issue any more. Thanks!

@blgm blgm closed this as completed Aug 6, 2021
@berlin-ab
Copy link

@onsi It seems like this would also be an issue for InterceptGomegaFailure(f func()) (err error) which relies on Default.(*internal.Gomega).Fail. I could be misreading that though.

@onsi
Copy link
Owner

onsi commented Nov 29, 2021

sorry for the delay on my end. Yes InterceptGomegaFailure would run into the same issue if called from a goroutine. you'll want to use a new Gomega and pass it a custom failure handler that aggregates failures for you. It's not a lot of code and I could imagine adding something to the DSL to make that use case easier but I'm heads down finishing up Ginkgo V2 right now.

@tklauser tklauser mentioned this issue Dec 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@berlin-ab @onsi @blgm