fix: lock suite.currentSpecReport.SpecEvents

[austince]

I'm not sure this is the right fix, but found a race condition when using By() concurrently.

WARNING: DATA RACE 12:52
Read at 0x00c000713248 by goroutine 51: 12:52
  github.com/onsi/ginkgo/v2/internal.(*Suite).processCurrentSpecReport() 
      /home/semaphore/git/go/1.21.7/pkg/mod/github.com/onsi/ginkgo/v2@v2.15.0/internal/suite.go:414 +0x7a 
  github.com/onsi/ginkgo/v2/internal.(*group).run()
      /home/semaphore/git/go/1.21.7/pkg/mod/github.com/onsi/ginkgo/v2@v2.15.0/internal/group.go:374 +0xd97
  github.com/onsi/ginkgo/v2/internal.(*Suite).runSpecs() 
      /home/semaphore/git/go/1.21.7/pkg/mod/github.com/onsi/ginkgo/v2@v2.15.0/internal/suite.go:489 +0x1291
  github.com/onsi/ginkgo/v2/internal.(*Suite).Run() 
      /home/semaphore/git/go/1.21.7/pkg/mod/github.com/onsi/ginkgo/v2@v2.15.0/internal/suite.go:130 +0x5f7 
  github.com/onsi/ginkgo/v2.RunSpecs()
      /home/semaphore/git/go/1.21.7/pkg/mod/github.com/onsi/ginkgo/v2@v2.15.0/core_dsl.go:300 +0xeaa
Previous write at 0x00c000713248 by goroutine 14849: 
  github.com/onsi/ginkgo/v2/internal.(*Suite).handleSpecEvent() 
      /home/semaphore/git/go/1.21.7/pkg/mod/github.com/onsi/ginkgo/v2@v2.15.0/internal/suite.go:284 +0x204
  github.com/onsi/ginkgo/v2/internal.(*Suite).By()
      /home/semaphore/git/go/1.21.7/pkg/mod/github.com/onsi/ginkgo/v2@v2.15.0/internal/suite.go:307 +0x1d7 
  github.com/onsi/ginkgo/v2.By()

[onsi]

hmm - can you share the code where you found this. sometimes that race condition indicates that your concurrent By is outliving the lifecycle of the spec in question

[austince]

sometimes that race condition indicates that your concurrent By it outliving the lifceycle of the spec in question

Hmm maybe that's it. Essentially we have a mock that registers a callback and is called concurrently by a process that is may or may not be stopped when the test is finished.

[onsi]

Right - you might need to fix that instead. One of the drawbacks of the DSL is that invoking it outside of the lifecycle of the intended spec can result in attaching information to the wrong spec.

[austince]

That makes sense -- thanks! Would the same issue occur with assertions that outlive the test lifecycle, or are those safely ignored?

[austince]

And last question -- would using a DeferCleanup to ensure these callbacks are deregistered have appropriate guarantees, or would we need to do it within the It block explicitly?

[onsi]

DeferCleanup would work. note that you’ll need to make sure it actually exits whatever goroutine it spun up before you exit the cleanup callback. sometimes these sorts of things don’t block when you tell them to stop. they just stop… eventually. that sort of thing could still leak between specs.

[austince]

Thank you!