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

Race condition introduced in By in v2.5.0 #1134

Closed
JoelSpeed opened this issue Feb 8, 2023 · 2 comments
Closed

Race condition introduced in By in v2.5.0 #1134

JoelSpeed opened this issue Feb 8, 2023 · 2 comments

Comments

@JoelSpeed
Copy link
Contributor

I believe db83f33 has introduced a race condition in the By utility in the core DSL.

In version 2.4.0, By was implemented directly into core_dsl.go and used AddReportEntry (which adds report untries under lock) and then GinkgoWriter.Println (which writes to the output under lock) to add the report entry and then write the output.

This meant that you could use the By utility within go routines within a test suite, if for example, you wanted to use a waitgroup to wait for multiple conditions in tandem.

In version 2.5.0, running tests with the ginkgo binary those same tests now report a data race because the By reporter is now writing events not under lock.

By is now implemented in suite.go and calls suite.handleSpecEvent which appends the event for the report entry under lock but then calls the reporter.EmitSpecEvent which in the DefaultReporter is causing a race when not used under lock.

Digging in, I believe the issue is because emit is writing changes to the DefaultReporter not under lock.

So either we need to adjust the handleSpecEvent to use the suite selective lock to emit the event to the reported under lock or we need to fix the DefaultReporter so that it isn't racy here

@JoelSpeed JoelSpeed changed the title Race condition introduced in v2.5.0 Race condition introduced in By in v2.5.0 Feb 8, 2023
@onsi
Copy link
Owner

onsi commented Feb 8, 2023

Welp! thanks for the detailed deep dive @JoelSpeed - I'll take a look!

@onsi onsi closed this as completed in 2d5075a Feb 13, 2023
@onsi
Copy link
Owner

onsi commented Feb 13, 2023

this is now fixed in 2.8.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants